Jep 430 String template - Grammar, AST and codegen changes

[jarthana]

Issue #544

What it does

Fixes #544

How to test

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[jarthana]

The failures in DietRecoveryTest are recovery related and I think we can simply adjust the tests here. However, the ones in NullAnnotationTest need attention. They arise because we are triggering initialization of null analysis and thus resolution of annotations much earlier. I..e at the time of computing default imports itself. Either we need to

1. ensure the null analysis is not prematurely triggered or
2. instead of adding an import, replace "STR" or "RAW" with their qualified names. (I haven't tried this though)

[srikanth-sankaran]

First batch of comments.

[srikanth-sankaran]

The production with Primary widens things, but may still leave us short of all possible expressions denoting a processor, but we can tackle this later.

[srikanth-sankaran]

A major recommendation is to:

1. Come up with a method jumpOverEmbeddedExpression modelled after jumpOverMethodBody (best not to completely duplicate this method, but define a customizable method jumpToMatchingBrace that can be called by both
2. This method should continue to maintain withoutUnicodeBuffer and withoutUnicodePtr as needed.
3. Maintain co-ordinates of fragments and expression relative to string start rather than relative to source, this way if we find unicodes and switch to alternate buffering, the already recorded offsets will still be valid.
4. The recorded offsets need to be string relative if withoutUnicodePtr == 0 and will simply be withoutUnicodePtr itself if the string has any unicodes

This, if can be made to fly can significantly simplify the implementation.

I know that is a big if 🤣

[jarthana]

A major recommendation is to:

1. Come up with a method `jumpOverEmbeddedExpression` modelled after `jumpOverMethodBody` (best not to completely duplicate this method, but define a customizable method `jumpToMatchingBrace` that can be called by both

I played around a bit. While it works well for most cases, it is confused with cases like this:
String s = STR."\{ STR."\{ STR."}abc{" }" }";

To make it work, we should make it aware of the nested {} too, that might make it more complicated than it is now.

[srikanth-sankaran]

A major recommendation is to:

1. Come up with a method `jumpOverEmbeddedExpression` modelled after `jumpOverMethodBody` (best not to completely duplicate this method, but define a customizable method `jumpToMatchingBrace` that can be called by both

I played around a bit. While it works well for most cases, it is confused with cases like this: String s = STR."\{ STR."\{ STR."}abc{" }" }";

To make it work, we should make it aware of the nested {} too, that might make it more complicated than it is now.

Can you pick up the changes from #1571 - this passes all tests and in this change jumpOverMethodBody calls scanForStringLiteral which should automatically handle the case you cite

[jarthana]

I noticed we report an explicit import of STR as unused import. This import should be marked as used or ignored.

[mpalat]

2. The DOM nodes need more thoughts on handling the string literals and text blocks that will not have the delimiters such as " or """.

capturing the essence of an offline discussion with @jarthana here [also to show to management that I am not whiling away time :) ]
(1) A list of nodes without regard to correctness is one option what we discussed. This is probably the simplest solution - leave the correctness questions [eg: is there an expression immediately after String etc] to the compiler.

(2) Alternatively, I feel, something akin to VariableDeclarationFragment may be suited for this. Essentially, this would be a StringDeclarationFragment having two properties - one for the string fragment and the other for the expression - the embedded expression - the { and } will have to be handled. There is a clear demarkation of what is String and What is Expression, so that setting can be done separately. Agree that there will be an odd man out, but this is not an issue -think of a variable without an initialiser. It would also mean continuity in source range [satisfying the source range verifier] which was one of the issues Jay raised.

• 2 cents.

[jarthana]

In the interest of getting this out to users and have it tested enough, I plan to push these changes tomorrow. I have taken out the DOM changes and plan to take it via #1537.
Other than the DOM part, what needs work is the code assist. I have tested that assist (including completion) works around string templates (but just not inside them).

[mpalat]

I have taken out the DOM changes and plan to take it via #1537.
@jarthana I guess you meant the PR number of the DOM changes?

[stephan-herrmann]

The failures in DietRecoveryTest are recovery related and I think we can simply adjust the tests here. However, the ones in NullAnnotationTest need attention. They arise because we are triggering initialization of null analysis and thus resolution of annotations much earlier. I..e at the time of computing default imports itself. Either we need to

1. ensure the null analysis is not prematurely triggered or
2. instead of adding an import, replace "STR" or "RAW" with their qualified names. (I haven't tried this though)

@jarthana I happen to see this just now. Does early triggering initialization of null analysis still happen with the released change? If so, could you point me to the code change causing this? "Doing things too early" is a constant concern in the implementation of null analysis ...

[jarthana]

NullAnnotationTest

I have added a kludge in CUScope, line number 772:
this.environment.globalOptions.isAnnotationBasedNullAnalysisEnabled = false;

If you comment out this code and run the NullAnnotationTest, you will see some tests failing.

[stephan-herrmann]

NullAnnotationTest

I have added a kludge in CUScope, line number 772: this.environment.globalOptions.isAnnotationBasedNullAnalysisEnabled = false;

If you comment out this code and run the NullAnnotationTest, you will see some tests failing.

@jarthana as this now hits in #2178 I took a brief look and see the failures in NullAnnotationTest at 21+ when the kludge is removed. Just the other day we launched #2209, because the order of compilation steps is still brittle after quite some efforts in this field. By resolving a field right during the very first step CHECK_AND_SET_IMPORTS, we are certainly not in a position to safely resolve things like annotations, but class StringTemplate is annotated. So that approach is asking for trouble indeed.

Since we don't even know if this implicit import StringTemplate#STR will survive beyond the current round of preview, I wonder how much we should invest her. Would it be fair to put the magic import under the preview flag for now?

If that's not good, my next idea would be to encapsulate all fields of ImportBinding, so that we can use lazy initialization, but that would affect all locations accessing this class, so not a tiny change.

Third option: add the magic import much later. This might be OK because we'll never need a resolved field for resolving supertypes :) -- when resolving annotations we could in theory need to see the field from smth like @Process(tmpl=STR) but that will hardly be a realistic case ... so perhaps we could find a safe point in time to set the import after the LookupEnvironment has been bootstrapped with core types and annotations (incl. meta annotations).

Speaking of it: @srikanth-sankaran , do you think an explicit phase for resolving some primordial things like @Target, @Retention, Object, Enum, String, StringTemplate in a well defined order could help?

[stephan-herrmann]

If that's not good, my next idea would be to encapsulate all fields of ImportBinding, so that we can use lazy initialization, but that would affect all locations accessing this class, so not a tiny change.

That's what I did in #2226. This makes resolving of StringTemplate.STR behave more like normal fields.

[srikanth-sankaran]

Since we don't even know if this implicit import StringTemplate#STR will survive beyond the current round of preview, I wonder how much we should invest her. Would it be fair to put the magic import under the preview flag for now?

Not unreasonable, but we may be postponing the problem.

Third option: add the magic import much later. This might be OK because we'll never need a resolved field for resolving supertypes :) -- when resolving annotations we could in theory need to see the field from smth like @Process(tmpl=STR) but that will hardly be a realistic case ... so perhaps we could find a safe point in time to set the import after the LookupEnvironment has been bootstrapped with core types and annotations (incl. meta annotations).

This sounds promising. We could defer it almost to the point where an error would otherwise surface.

Speaking of it: @srikanth-sankaran , do you think an explicit phase for resolving some primordial things like @Target, @Retention, Object, Enum, String, StringTemplate in a well defined order could help?

Perhaps, but needs deeper study.