Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only cover scope boundaries in the parent layer if there is a scope boundary in the injected layer #19726

Merged
merged 3 commits into from Jul 30, 2019

Conversation

@nathansobo
Copy link
Contributor

commented Jul 30, 2019

Fixes #19582

#19556 introduced new behavior for scopes from injected grammars, causing them to "cover" or "shadow" scope boundaries in the parent grammar. This was added to prevent duplicated and conflicted scope boundaries from being rendered in cases where the syntax tree of a parent grammar contains nodes that overlap with the syntax tree of the child grammar. See the body of #19556 for more details.

In #19582, we discovered an edge case in the injection of the JSDoc grammar into JavaScript block comments. In some cases, the HighlightIterator associated with the nested JSDoc grammar ends up getting parked at the end of the comment but not getting associated with any openTags or closeTags. In that scenario, the HighlightIterator continues to shadow the scopes from the parent grammar even though it is not expressing a scope boundary itself. This causes the close tag for the comment to be hidden, incorrectly extending the comment scope beyond the bounds of the actual comment block.

The solution in this PR is to only hide scopes from the parent grammar in situations where the child grammar's HighlightIterator has at least one close tag or open tag. This prevents the close of the comment scope from being obscured and solves #19582.

In the process of exploring this issue, it occurred to @maxbrunsfeld and I that there might be more diabolical cases where a node in the child grammar shadows only a close tag in the parent grammar. However, this actually seems unlikely. If we shadow a scope close at a given position, there's a high probability that we also shadow the scope open due to the structure of the syntax tree, thus preventing an unbalanced scope open from occurring. In the event we encounter an actual case of an unbalanced tag caused by shadowing, we will have more information on which to base a solution. In the meantime, the hypothetical possibility doesn't justify the complexity of a full solution.

Antonio Scandurra and others added some commits Jul 29, 2019

@nathansobo nathansobo requested review from maxbrunsfeld and as-cii Jul 30, 2019

@nathansobo nathansobo changed the title For language layers, only cover scope boundaries in the parent layer if there is a scope boundary in the injected layer Only cover scope boundaries in the parent layer if there is a scope boundary in the injected layer Jul 30, 2019

Antonio Scandurra
@as-cii

as-cii approved these changes Jul 30, 2019

Copy link
Contributor

left a comment

Thanks for picking this up and pushing it over the finish line, @nathansobo! I fixed a few linter errors in a467402 to make CI green.

It seems like in the current situation we sometimes allow for a shallower grammar to emit scopes even when a deeper grammar is parked at the same position. If I understand correctly, this may lead to one of those diabolical scenarios you and @maxbrunsfeld illustrated in the body of this pull request in which we could close a tag that was never opened (or vice versa).

If that's correct, I wanted to mention a couple of observations (which we may want to address later, if ever at all) about the invariants of the injection system. I am not super familiar with this mechanism in general, so pardon my ignorance on the subject. 🙏

  • I am a bit unclear on why the injected grammar is reporting a boundary at the end of the injection point ([1, 2] in the test introduced with this pull request) even if no scopes are being reported there. Could we make the underlying iterator not report such position at all when scopes needn't be yielded? If we do so, does it mean we could avoid the extra conditional in the HighlightIterator's orchestration logic?
  • Alternatively, could we change the injection mechanism to always report the parent's scopes at the beginning and end of an injection, but skip everything in between? This would seem to work well for constructs such as Markdown code-blocks or the JsDoc example in this pull request, but I am unclear on whether it would be something that works without loss of generality.

Either way, this seems like a reasonable workaround to me and I am totally cool with shipping it as it is until we gather more real-world examples to drive a more general solution. 🚢

@maxbrunsfeld
Copy link
Contributor

left a comment

Sorry for introducing this gnarly issue. Thanks for diving in and fixing it.

@@ -1010,7 +1010,8 @@ class HighlightIterator {
if (
next.offset === first.offset &&
next.atEnd === first.atEnd &&
next.depth > first.depth
next.depth > first.depth &&
next.openTags.length + next.closeTags.length > 0

This comment has been minimized.

Copy link
@maxbrunsfeld

maxbrunsfeld Jul 30, 2019

Contributor

Oh wow you ended up with a really tiny diff! I think this fix makes sense.

Down the road, if this type of problem occurs in other cases, we'll probably just have to make this conditional even more specific.

@nathansobo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Sorry for introducing this gnarly issue.

You introduced a lot of good stuff in the process so no need to apologize. Thanks for your time yesterday talking it through.

@nathansobo nathansobo merged commit cac1a77 into master Jul 30, 2019

1 check passed

Atom Pull Requests #20190730.2 succeeded
Details

@nathansobo nathansobo deleted the as-ns/fix-injected-grammars branch Jul 30, 2019

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

Just some additional thoughts on this:

Recap - Right now, we "cover up" scope start events from shallower layers if a deeper layer has a scope start event at the same position, and similarly for scope end events. This PR fixes a problem where we were incorrectly covering up a scope end event that should not have been covered.

Remaining Error Case - It's still theoretically possible that we might incorrectly "cover up" a closing scope, leading to a similar kind of scope "spillage" that happened in #19582. It could happen in a situation like this diagram, in which curly braces represent scope boundaries:

layer-1: a{b c}
layer-2: a b{c}

In the outer layer (layer-1) a scope contains b c, but in the inner layer (layer-2) a scope contains just c. We would not cover the scope start event before b in layer 1, but we would cover the scope end event after c, because layer-2 also has a scope ending there.

Possible Solution - I think that to solve this (theoretical) problem, we would need to further restrict the criteria for when to "cover up" outer layers' scope boundaries. We should only cover up scope events if the two layers are parked on syntax nodes with the same range (i.e. first.treeCursor.startOffset === next.treeCursor.startOffset && first.treeCursor.endOffset === next.treeCursor.endOffset.

I think that this would guarantee that we would only cover closing scopes if we also covered up the opening scopes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.