Fix tree-sitter-language-mode null highlight iterators #22080
Conversation
Do you want to backport it now? TBH, Atom dev is not reliable at the moment to fix things against. The failing tests seem to be related to something else. |
|
There should be a v1.56.0-beta1 release with all of these fixes since beta is currently very buggy. |
There was a problem hiding this comment.
I applied this change against one commit before bumping Electron, and I am testing it.
| getCloseScopeIds() { | ||
| return []; | ||
| } | ||
| } |
There was a problem hiding this comment.
i do not understand the fix, but i assume that if you want to fix it and avoid code duplication, you can also do:
class NullLanguageModeHighlightIterator extends NullLayerHighlightIterator {
seek() {
return [];
}
}And should work the same.
There was a problem hiding this comment.
Sure. Could also use mixins or whathaveyou. However, the actual (non-null) iterators are not actually related beyond sharing method names, and are used entirely separately (i.e. not intended to be interchangeable in any meaningful way). So I'm a little apprehensive about making null- iterators dependent on each other's implementation. It might create similar issues going forward.
There was a problem hiding this comment.
So at this point in the list possible drawback the code duplication is not a drawback at all if the code is intended to behave differently.
There was a problem hiding this comment.
I'm on the fence about this. On one hand, there is apparent code duplication. On another hand, null iterators mimic non-null iterators structurally (i.e. both null and non-null iterators use separate implementations). Non-null iterators behave very differently and share very little code beyond method names. Null iterators are pretty similar however, but arguably that's a coincidence, and a shared implementation is what basically caused #22078 in the first place.
Overall, I'm leaning to "there's not so much code duplication as to really worry about it, and there are potential drawbacks to reducing it".
I should've been more clear about the reason for doing it the way I did in the OP, but I was in a bit of a hurry when I made this PR.
aminya
left a comment
aminya
left a comment
There was a problem hiding this comment.
Could you update the branch? The tests appear to be fixed on the master.
lierdakil
force-pushed
the
fix-tree-sitter-highlight-iterator-2
branch
from
b1cb6a5 to
2219c74
Compare
|
Rebased |
5 participants
Requirements for Contributing a Bug Fix
Identify the Bug
#22078
Description of the Change
There are two types of highlight iterators in the file -- one for the LanguageMode, where
seekmethod returns an array, and one for Layer, which returns a boolean(-ish). Both usedNullHighlightIteratoron occasion, which led to some breakage: one instance was fixed in #21753, however that in itself created #22078. This PR disambiguates the two kinds of iterators and uses a separate class in place ofNullHighlightIteratorwhere appropriate.Alternate Designs
An alternative approach is making both kinds of iterators conform to a single interface, i.e. for
seekto either return an array or a boolean. However, that is much more involved and probably entails a complete rewrite of some parts of the code. I do not understand the code well enough to attempt this, and I don't really have the time to do it either.Possible Drawbacks
One obvious drawback is some minor code duplication. I'm on the fence about this.
On one hand, there is apparent code duplication. On another hand, null iterators mimic non-null iterators structurally (i.e. both null and non-null iterators use separate implementations). Non-null iterators behave very differently and share very little code beyond method names. Null iterators are pretty similar to each other however, but arguably that's a coincidence, and a shared implementation is what basically caused #22078 in the first place.
Overall, I'm leaning to "there's not so much code duplication as to really worry about it, and there are potential drawbacks to reducing it".
Verification Process
I've stared at the code for about half an hour. That was about enough to understand something is horribly wrong there.
No tests were performed as of the time of the writing, but I'm pretty confident in the results (the issue is pretty obvious to begin with). If/when the tests are performed, I will update this text.@aminya ran CI, one unrelated test failed on Windows due to a timeout, and prettier complains about some formatting on line 1544, which is not touched in this PR. He also verified that #22078 is indeed fixed by this PR.
Release Notes
N/A, this issue at present manifests only in v1.56.0-beta0 and up.
Notice
⚠️ 🚨⚠️ 🚨⚠️ 🚨⚠️ This should be backported to v1.56 branch. Since the code didn't change at all between v1.56 and master, the backport should be very straightforward. I can make a PR against 1.56-releases branch if that would be suitable.