[Breaking]: Visibility of fields/properties/classes does not match upstream tm4e implementation

[udlose]

Warning

Breaking API change

I'm working on fixing this now as we discussed here. I'm creating this issue to document the changes here.

Note

I didn't examine other folders/files for similar discrepancies so there may be other changes needed.

The upstream java implementation declares StateStack (and some other classes in the .internal.grammar package as final (sealed in .NET). So if you wanted to have parity with the upstream impl, we should include a breaking change by declaring these classes as sealed. Also, notice it's in the .internal.grammar package, suggesting it's not meant for public extension anyways. Implementers could still implement IStateStack if desired.

Here is a list of upstream differences I found - I only looked in this folder/namespace. So, there may also be others:

class name	sealed/unsealed (has parity with TextMateSharp)	class visibility (has parity with TextMateSharp)
AttributedScopeStack	sealed ❌	internal ❌
BalancedBracketSelectors	sealed ❌	public ✅
BasicScopeAttributes	sealed ❌	internal ❌
BasicScopeAttributesProvider	sealed ❌	internal ❌
Grammar	sealed ❌	public ✅
LineTokenizer	sealed ❌	internal ❌
LineTokens	sealed ❌	internal ❌
TokenTypeMatcher	sealed ❌	internal ✅
TokenizeLineResult	sealed ❌	internal ❌

[udlose]

I don't know how important the following is, so just lmk if it isn't a big deal...cool? Just wanted to bring it to your attention if you weren't aware....

@danipen I was looking at the parity of some of the classes under https://github.com/eclipse-tm4e/tm4e/tree/main/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/rule folder.

I was comparing the CaptureRule java class:

public final class CaptureRule extends Rule {

	public final RuleId retokenizeCapturedWithRuleId;

	CaptureRule(final RuleId id, final @Nullable String name, final @Nullable String contentName,
			final RuleId retokenizeCapturedWithRuleId, final @Nullable String grammarScope) {
		super(id, name, contentName, grammarScope);
		this.retokenizeCapturedWithRuleId = retokenizeCapturedWithRuleId;
	}

	@Override
	public void collectPatterns(final IRuleRegistry grammar, final RegExpSourceList out) {
		throw new UnsupportedOperationException();
	}

	@Override
	public CompiledRule compile(final IRuleRegistry grammar, final @Nullable String endRegexSource) {
		throw new UnsupportedOperationException();
	}

	@Override
	public CompiledRule compileAG(final IRuleRegistry grammar, final @Nullable String endRegexSource,
			final boolean allowA, final boolean allowG) {
		throw new UnsupportedOperationException();
	}
}

to your CaptureRule class:

    public class CaptureRule : Rule
    {
        public RuleId RetokenizeCapturedWithRuleId { get; private set; }

        public CaptureRule(RuleId id, string name, string contentName, RuleId retokenizeCapturedWithRuleId) : base(id, name, contentName)
        {
            RetokenizeCapturedWithRuleId = retokenizeCapturedWithRuleId;
        }

        public override void CollectPatternsRecursive(IRuleRegistry grammar, RegExpSourceList sourceList, bool isFirst)
        {
            
        }

        public override CompiledRule Compile(IRuleRegistry grammar, string endRegexSource, bool allowA, bool allowG)
        {
            return null;
        }
    }

I'm already fixing your CaptureRule class by making it sealed and changing the ctor to internal. But I also noticed diffs in the methods:

1. Even though there is no implementation in the upstream, you don't have a CompileAG method. Should I add one?
2. The upstream's Compile methods throws UnsupportedException, but yours returns null. Should this throw NotImplementedException instead of returning null?

I know there are others that have the method discrepancies but want to get your thoughts on how tight the parity should be.

[danipen]

When I originally ported the code, I did it as a fairly direct line-by-line translation from the Java implementation so the structure matched closely.

However, since then the upstream Java code has continued to evolve and those changes haven’t always been ported over.

So at this point I treat the upstream implementation more as a reference than a strict mirror. The goal is to keep behavioral compatibility where it matters, but not necessarily maintain a 1:1 API surface if it isn’t needed by the C# implementation.

Regarding the specific points:

• If CompileAG is not actually required by the current flow in our implementation, I wouldn’t add it just for parity.
• Returning null vs throwing NotImplementedException was mostly a pragmatic choice at the time since the method should never be hit in practice.

So in general: parity where it affects behavior is important, but we don’t need to mechanically replicate every upstream change unless it impacts functionality.

[udlose]

ok that helps and makes sense. thanks for the clarification!