Introduce token modes - #127
Conversation
msujew
left a comment
There was a problem hiding this comment.
Nice! Can you resolve the merge conflict so we can check the performance in the GitHub CI? Locally, it seems to look good. Only a 2-3% loss on the statemachine example, which is acceptable.
|
@msujew I discovered a problem with my main grammar. The problem arises when I create a token mode and start to create token declarations within this token mode. As soon as I complete the word "token", the language server seems to end up in a endless loop. It does not react anymore. After completing the declaration and restarting the LS, it works. So the problem is during editing incomplete grammars :-/... The rule If I would refactor the grammar to have no common prefix in the subrules of Before I refactor it would be good to discuss whether these members have a good syntax or whether we should alternate it (which could remove the common prefix automatically). |
|
Ideas... Some quick fixes or code actions come into my mind:
Or
|
467be1d to
2996cda
Compare
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 1ee2bcd | Previous: d803417 | Ratio |
|---|---|---|---|
BenchmarkWorkspaceCycle (typefox.dev/fastbelt/examples/statemachine) - MB/s |
11.92 MB/s |
5.21 MB/s |
2.29 |
This comment was automatically generated by workflow using github-action-benchmark.
No, it should fail out of the box. I can also complete this line as you started. But afterwards EDIT: I found out how to debug. Some kind of endless-loop in entered in this case. EDIT: Fixed the endless loop and another issue with the help of @msujew |
Lotes
left a comment
There was a problem hiding this comment.
Added some findings and also questions from my side.
msujew
left a comment
There was a problem hiding this comment.
Really solid contribution. Thanks a lot!
I noticed another few missing validations that we should add as part of this PR:
- Duplicate keyword/token definitions in the same token mode should result in an error. I.e. you can currently write
token mode default { "x" "x" }. - Similarly Token references should be unique in the same token mode. Right know, you can write something like
token mode default { hidden WS comment WS }without a validation error appearing. - When a token is declared, but not referenced in any token mode, the diagnostic appears on the first usage site. I would expect it on the token declaration itself. Also, similar to the keyword issue, this should be an error.
- A non-default token mode without a
popcommand should show a diagnostic.
# Conflicts: # internal/grammar/types_gen.go # Conflicts: # internal/grammar/types_gen.go
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io> # Conflicts: # lexer/lexer.go
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
17d7c0e to
557bdc7
Compare
Signed-off-by: Markus Rudolph <markus.rudolph@typefox.io>
msujew
left a comment
There was a problem hiding this comment.
Looking closer at some of the logic, I found some issues with some specific constellations. Works much better in general now, though!
| hidden token WS: /\s+/ | ||
| comment token ML_COMMENT: /\/\*[\s\S]*?\*\// | ||
| hidden token SL_COMMENT: /\/\/[^\n\r]*/ | ||
| hidden token SL_COMMENT: /\/\/[^\n\r]*/ No newline at end of file |
There was a problem hiding this comment.
Formatting: You've removed the EOL here.
| case grammar.TokenGroupUsage: | ||
| tokenGroup := member.Group() | ||
| tokenIndex := result.TokenIndex.ByTokenGroup[tokenGroup] | ||
| pushTokenTypeUsage(tokenIndex, tokenGroup.Modifier(), tokenGroup.Command()) |
There was a problem hiding this comment.
Suggestion: Token groups in token modes show some buggy behavior. Let's take the following grammar code:
token group X {
"x"
}
token mode default {
"x" -> push(Y)
}
token mode Y {
X -> pop
}
Giving a grammar like this the input xx actually results in an error at the second x, because the X token group does not have start char hints (or is lexible at all). Using X inside a token mode, should actually add its "sub-tokens" to the token mode.
| //if we are stuck on the same token, consume it and try to move on | ||
| if parserState.lastErrorIndex == parserState.Index { | ||
| parserState.Index++ | ||
| } |
There was a problem hiding this comment.
Suggestion: This should not bump the Index beyond parserState.Length. Otherwise we run into issues at EOF.
| } | ||
|
|
||
| for _, token := range tokens.TopLevel { | ||
| if _, ok := token.Content().(grammar.RegexpTokenElement); ok { |
There was a problem hiding this comment.
Suggestion: This drops token usage on tokens like hidden token Test: "test".
|
|
||
| TokenDecl: Modifier=TokenModifier? "token" Name=ID ":" Content=TokenElement Command=TokenCommand? ";"? | ||
|
|
||
| interface TokenElement {} |
There was a problem hiding this comment.
WDYT about this name? Similar RegexpTokenElement -> RegexpTokenContent etc.
| interface TokenElement {} | |
| interface TokenContent {} |
| } | ||
|
|
||
| func (s *scopeProviderImpl) ScopeRuleCallRule(ctx context.Context, reference *core.Reference[AbstractRule]) core.Scope { | ||
| root, _ := reference.Owner().Document().Root.(Grammar) |
There was a problem hiding this comment.
Suggestion: Don't discard the ok of the cast. Can theoretically lead to panics here.
| func mergeImports(target *map[string]bool, source map[string]bool) { | ||
| for imp := range source { | ||
| (*target)[imp] = true | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Use maps.Copy for this. Also *map is not necessary. map is always a pointer type.
| severity, | ||
| fmt.Sprintf("The token '%s' is never referenced in a parser rule, so the lexer can never produce it.", tokenRef.Name()), | ||
| member, | ||
| core.WithTextRange(member.TextRange()), |
There was a problem hiding this comment.
Suggestion: NewDiagnostic automatically takes the TextRange() value to initialize the range. No need to use WithTextRange again.
| if !seen.Has(terminal.Name()) { | ||
| accept(core.NewDiagnostic( | ||
| severity, | ||
| fmt.Sprintf("The token '%s' is never referenced in a parser rule, so the lexer can never produce it.", terminal.Name()), |
There was a problem hiding this comment.
Can we move this string somewhere? It's repeated 6 times in this method.
| } | ||
|
|
||
| func generateNestedString() (string, error) { | ||
| content := "" |
There was a problem hiding this comment.
Suggestion: Use a strings.Builder instead of a plain string here. Concatinating a new string 10000 times is quite expensive.

Closes #50
push(name)to push a token mode onto the token mode stackpopto pop the topmost token modemode(name)to set the topmost token modeKeywordSelector