kotlin
kotlin copied to clipboard
JetBrains
KT-51714: Parsing fails for context receivers with incomplete label
When parsing the following or similar code an AssertionException is raised:
context(@)
fun a() {}
Inside the parser parseLabelDefinition() assumes, that it is currently at an IDENTIFIER. The corresponding condition isAtLabelDefinitionOrMissingIdentifier(), however, also accepts when parser is at an @ Symbol. This trips the assert.
With this change, a proper parse error will now be shown. The message might be a bit too descriptive, though.
Incredible, that I overlooked the existing ticket. Youtrack seemingly only suggests the other ticket, if the error message is in the title, but not if it's in the description.
Hi!
I'm really sorry for the review delay, somehow I overlooked the PR.
The changes in parser looks good, but there's an issue that effectively all the fixes that turn red code to green may be enabled since the next major version and should be controlled via compiler settings, i.e. even when using 1.8 compiler one may want to have -
Xlanguage-versionequal to 1.7 and the code from the issue should be red in that case.For non-parsing features, you may just add another entry to
org.jetbrains.kotlin.config.LanguageFeatureand implement relevant support for it (see usages of some of the existing entries), but Kotlin parser since it might be used in version-agnostic environment (e.g. indexing), thus for new parsing features we add aDeclarationCheckerimplementation that would check if there's been a..>= *initializer*case and report a special diagnostic when language feature is disabled. So, the thing that previously was a syntax error, would become a diagnostic fromorg.jetbrains.kotlin.diagnostics.ErrorsWhile it's indeed looks rather complicated, we have to maintain the contract that when the compiler declares it supports 1.7 it should compile some file if and only if some previous 1.7-supporting compiler did compile it.
UPD: All of the above is indeed for #4771
I take it, that, given the ..>= *initializer* part, the comment was suppose to go into #4771.
I completely understand the reasoning behind the version compatibility issue and I'll change it over there.
Is it also necessary in the case of this PR? My guess would be no. Context-Receivers are going to be in preview in 1.7, so opting-in is required in the first place.
Oh, that commend was indeed about #4771 :(
Regarding this PR, thank you for initiating the process, but I think it's worth trying something like that because isAtLabelDefinitionOrMissingIdentifier is being called each time before parseLabelDefinition and expecting single @ doesn't make much sense to me.
I'm going to push my commit above without any test because pushing test to the repository would make project indexing failing until all the team updated to the new bootstrap. After that, I would merge your tests if you don't mind.
I guess, expecting a single '@' was once intended to improved the error handling by hinting that the label is missing an identifier. Such logic was never implemented in parseLabelDefinition.
In most cases the @ is interpreted as the start of an annotation. Here the compiler provides appropriate hints, if the '@' is not followed by an identifier.
However, in the case of a context receiver, there cannot be an annotation in front of it. Without my changes, it would simply say something in the lines of "unexpected token @". So while it is being overshadowed by annotations in most cases, it would still provide useful information in those other cases.
@kralliv Nice observation! I'll merge your change then, but still will split in two phases manually
Alright. Yeah, the split is really for the better. Testing this was a bit painful with Intellij erroring all the time. Others don't have to feel that pain too :)