More systematic completion for all kinds of case labels

[stephan-herrmann]

#1312 tries to fix a regression in completion after case, which was caused by recent work concerning Java 17.

Looking at the code changes, however, I don't get a clear picture of the various styles of case and what kinds of completions should be proposed for each.

Hence I would like to see this area captured more systematically, before we continue tweaking issues back and forth.

[stephan-herrmann]

Scheduling for 4.30 M3, at which time we need to decide if a systematic approach is ready for prime time, or alternatively merge the regression fix (#1312) now, and continue after the release.

[stephan-herrmann]

@gayanper do you want to join me in this effort? Perhaps @mpalat could help identifying relevant use cases?

[gayanper]

Linking #2260

[stephan-herrmann]

I see the following list of questions:

1. what are the different kinds of switches that determine which case constructs are legal?
2. how can we discriminate those kinds in possibly incomplete code?
3. then per each of these kinds: what are the things that can possibly follow the case keyword?
4. can/should we propose more than just one token?

I suggest we address these questions top to bottom, and at least for those first questions @mpalat is our expert, right? :)

[gayanper]

I see the following list of questions:

1. what are the different kinds of switches that determine which case constructs are legal?
2. how can we discriminate those kinds in possibly incomplete code?
3. then per each of these kinds: what are the things that can possibly follow the case keyword?
4. can/should we propose more than just one token?

I suggest we address these questions top to bottom, and at least for those first questions @mpalat is our expert, right? :)

If I try to answer,

1. If the switch variable has a primitive type binding including String, we should try to suggest matching constants for that type in case. But at the same time the user might need to have use a qualification, which should also allow to do a SingleType completion as well. So one could write Constants.NAME as a case construct using code completions which complete both the qualifier, and then the constant.

2. Today the completion is handled at completionOnSingleNameReference when trying to complete on a case

3. So here we could use something similar:

• If the type is primitive or java.lang.String then we follow the logic above 1
• If the type is a Enum, we provide enum literals which does happens today
• If the type is a Class (not java.lang.Object) or an Interface, we could just let the Type completions cover listing all types or we could only provide the subtypes by being bit more smart. But trying to find subtypes might come with a cost based on the type hierarchy size.

4. What do you mean ?

[gayanper]

I think given the all pattern matching features, such as extracting values out of records as part of pattern matching, i think if would me better to handle all such switch case related logic in a separate handler rather than the current one. So that will give us the opportunity add more better completions for java record pattern recognition scenarios

[gayanper]

https://inside.java/2023/11/06/sip087/

[gayanper]

@stephan-herrmann @mpalat #2265 this is a WIP solution I started on and updated based on above points from @stephan-herrmann.

Let me know what you think ? May be we can start building on this or any other solution you think will be better and scalable.

[stephan-herrmann]

Thanks, @gayanper for your reply.

I'm a little confused, though, as the answers don't seem to exactly match to my questions :)

1. If the switch variable has a primitive type binding including String, we should try to suggest matching constants for that type in case. But at the same time the user might need to have use a qualification, which should also allow to do a SingleType completion as well. So one could write Constants.NAME as a case construct using code completions which complete both the qualifier, and then the constant.

This seems to answer (3) for one specific kind of switches.

2. Today the completion is handled at completionOnSingleNameReference when trying to complete on a case

I didn't yet ask about any location in code :)

This comes close to answering (1) :

3. So here we could use something similar:

• If the type is primitive or java.lang.String ...
• If the type is a Enum ...
• If the type is a Class (not java.lang.Object) or an Interface ...

4. What do you mean ?

Two ideas (there might be more opportunities):

• take the example of an enum switch: instead of case MyConstant, should we propose case MyConstant: or case MyConstant ->?
• when a record pattern is legal, should we propose the full pattern instead of just the record name?

As I recomment to include @mpalat in this discussion, we may have to wait a few days. his avatar says "On vacation - Back on March 7th" - I have an inkling that he may not be back before April 7th.

https://inside.java/2023/11/06/sip087/

Might as well use a source closer to home: https://manojnp.medium.com/record-patterns-building-on-records-d9370b19165f 😄 or https://www.youtube.com/watch?v=C1lAfolOJ6c for that matter (would have loved to introduce you to each other during that event).

[stephan-herrmann]

I think given the all pattern matching features, such as extracting values out of records as part of pattern matching, i think if would me better to handle all such switch case related logic in a separate handler rather than the current one. So that will give us the opportunity add more better completions for java record pattern recognition scenarios

If by "separate handler" you mean a new method inside CompletionEngine, then for sure this makes much sense.

[gayanper]

@stephan-herrmann Sorry for confusing, I think may be I didn't Cleary understood the different expectation in your questions. But hope one way or other I managed to provided answer from my perspective.

Yes we can wait for @mpalat and see what he thinks and how we should handle this as well. I know that Record pattern matching could open up more and more completions considering the record component extraction (destructor).

I hope we will be able to support current and future Record patterns without needing the change the parser and introduce more CompletionOn* nodes to handle :).

[gayanper]

@stephan-herrmann @mpalat #2265 this is a WIP solution I started on and updated based on above points from @stephan-herrmann.

Let me know what you think ? May be we can start building on this or any other solution you think will be better and scalable.

@mpalat @stephan-herrmann The reason I made this interim code change is for us to start out discussion around this and as well as for me to get going in my daily driver Eclipse + EclipseLS when using switch patterns ;)

[gayanper]

I just used the NB.LS in vscode for java code editing to see how it handled pattern matching. Now it works like this

Given the code

                    public final record LessThan(RangeValue lt) implements RangeOp {
                        
                    }

                    switch (op) {
                        case L
                    }

completions behind case L provide the type completion LessThan record type.

                    switch (op) {
                        case LessThan
                    }

compilations behind case LessThan provide the deconstruction pattern as LessThan(RangeValue rv)

cc: @stephan-herrmann

[mpalat]

As I recomment to include @mpalat in this discussion, we may have to wait a few days. his avatar says "On vacation - Back on March 7th" - I have an inkling that he may not be back before April 7th.

Nope, I wasn't on vacation but caught up with a lot of internal stuff - could not get to this.. And thanks to Stephan for pointing out the stale vacation date. And I see many questions already answered - so that could be good strategy :) Trying to answer the following:

"""
Two ideas (there might be more opportunities):

take the example of an enum switch: instead of case MyConstant, should we propose case MyConstant: or case MyConstant ->?
"""
Only check should be how are the rest of the case arms? If they are : then its : else ->. This excludes the case where this is the first arm, I would say either : or -> should be fine.

"""
when a record pattern is legal, should we propose the full pattern instead of just the record name?
"""
Am assuming that we are switching based on an interface I which among other classes have some records also implementing the same. Record name proposal will be only a type pattern and will not be a deconstruction pattern as we know.

Case 1: There are other case arms already and we are proposing somewhere in the middle
(a) If there are no record patterns, only classes, propose the type pattern ie case R r ->, and also the deconstruction patterns (subject to dominance rules).

(b) If there are record patterns already, then I would say propose deconstruction patterns first and then the type pattern.

Either cases, the proposals would be the same, only the priority can be different.

[gayanper]

@mpalat i will try to build a mvp solution in my poc PR and attach a screencast here. Then we can go from there to fine tune it. I can help with coding where you and @stephan-herrmann can help me reviewing code and the expectations.

I think we might need to make sure the same approach can be extended to plan java 23 patterns as well.

[stephan-herrmann]

Both of you are already miles ahead without answering my first question :) perhaps this is because you consider the answer as trivial, but is it?

1. what are the different kinds of switches that determine which case constructs are legal?

Does completion need to know whether we are looking at a switch statement or a switch expression?

Let's for now assume that looking at only switch(v) (with resolved bindings) will tell us enough for computing proposals (to be revisited later).

Easy stuff first: if v has a primitive type, there's not much we can do (and hopefully existing completion covers the few options, like proposing true and false).

If v has type String, does the Java 7 way of doing string switches suffice? A look into SwitchStatement.resolve() reveals, even in this case we could be in a non-traditional switch! Tricky!

Next if v has an enum type, does proposing enum constants suffice? What happens if the enum implements an interface, would that enable the use of other switch formats for that enum? (SwitchStatement.resolve() has no provision for that, so is JLS explicit in this?)

Finally, we have "all other reference types". Would this open up the switch for all kinds of patterns? What if the type of v is final, or any regular class (non-record, non-enum), or a wrapper class? Here be dragons. Let's chart them before we enter their territory! 😄

[gayanper]

Agree so many things, So I think I agree with @stephan-herrmann we need to first agree on what we want to support to start with and when I start look at expand my POC to reach similar what is offered by Netbeans. It feels like we are trying to open a Pandora's box.

for example:

interface Foo {}

record ImplementingRecord(String c1, int c2){}

switch (interface-type) {
   case ImplementingRecord
}

Now in such scenario we might need to introduce a new completion type for patterns, so that we can provide

• ImplementingRecord r
• ImplementingRecord(var c1, var c2)
• ImplementingRecord(String c1, int c2)

Now to support this, the JDT.UI must be changed as well, And if the developer needs symbol information on the destructor, then that needs to be implemented as well. At least now the selection parser doesn't seem to identify something like case ImplementingRecord() to provide parameter information.

Now if the developer wants to perform completions after ImplementingRecord(

switch (interface-type) {
   case ImplementingRecord(
}

to define the pattern by them self combining var and concrete types, then the completion engine lacks context. There is no information about we are in a switch in above scenario other than we are completing on a method invocation. So if we want to support this second scenario, we may beed to do some changes in NodeDetector as well.

[gayanper]

While waiting for answer from @mpalat for @stephan-herrmann 's question, I did some hacking and patch VSCode Java to use the new changes. It's still basic and we can refine this further if we want.

JDT.Type.Patterns.mov

[mpalat]

Both of you are already miles ahead without answering my first question :) perhaps this is because you consider the answer as trivial, but is it?

1. what are the different kinds of switches that determine which case constructs are legal?

• I think this is becoming question with increasing difficulty - in the light of JEP 455 where even an expression with primitive type can result in a case with patterns - so a blanket "primitive type => no case patterns" rule will not be true.
• case null should be valid if its a type switch and invalid in case of primitive types.
• Dominance is another factor that should be considered. is case Number is not legal after case Double for eg.

Does completion need to know whether we are looking at a switch statement or a switch expression?

• I believe this is not an influencing factor - every case arm valid for statement is valid for switch and vice versa.

Let's for now assume that looking at only switch(v) (with resolved bindings) will tell us enough for computing proposals (to be revisited later).

Easy stuff first: if v has a primitive type, there's not much we can do (and hopefully existing completion covers the few options, like proposing true and false).

• yes in present, no in future - ref JEP 455 -[Primitive Types in Patterns, instanceof, and switch (preview)]. https://openjdk.org/jeps/455

If v has type String, does the Java 7 way of doing string switches suffice? A look into SwitchStatement.resolve() reveals, even in this case we could be in a non-traditional switch! Tricky!

• I believe this part I've answered in my earlier comment - we should be giving options pertaining to both but with prioirity

Next if v has an enum type, does proposing enum constants suffice? What happens if the enum implements an interface, would that enable the use of other switch formats for that enum? (SwitchStatement.resolve() has no provision for that, so is JLS explicit in this?)

• I don't have an immediate answer to this.

Finally, we have "all other reference types". Would this open up the switch for all kinds of patterns? What if the type of v is final, or any regular class (non-record, non-enum), or a wrapper class? Here be dragons. Let's chart them before we enter their territory! 😄

• It will open the switch for all kinds of patterns and will be increasing in terms of new pattern types as and when they are supported. In case v is sealed, we still have some hope :) - we need to show only the permitted types. If it is a regular non-sealed final class/interface or if it is a record with different parameters, then we will have a field day - we can take the approach similar to what we would show a parameter, types that are accessible.

[srikanth-sankaran]

I offer to work on this for a comprehensive fix for the 4.35 M1 milestone

[stephan-herrmann]

I offer to work on this for a comprehensive fix for the 4.35 M1 milestone

I already see @gayanper 's thumbs-up, so I hope his efforts in #2265 will not be wasted (once again sorry for my slowness in reviewing).

One observation from that MR: for introducing a new proposal kind for type patterns (which I think would be great!) we also need a corresponding enhancement in jdt.ui. It would be good to see s.o. sign up for that part as well.

[stephan-herrmann]

Should the work here, by any chance, require non-trivial changes in the completion parser, let's please first try to get rid of 10-year old black-magic, by first addressing #1045 😎

[srikanth-sankaran]

Should the work here, by any chance, require non-trivial changes in the completion parser, let's please first try to get rid of 10-year old black-magic, by first addressing #1045 😎

I am for grammar restructuring if it will eliminate the need for all those special tricks deployed to parse Java 8.

I see those as techniques highly localized, documented, well understood and most importantly articulable in a lucid clear manner to whoever wants to understand.

Do they deserve the moniker black magic - I don't know.

As opposed to the admittedly-poorly-documented-and-hacky-and-cobbled-together-in-the-last-minute fallbackToSpringForward - that no doubt qualifies as black magic. And one that caused actual grief.

Or is there an episode of grief behind those grammar tricks that I have not been made aware of ??

[srikanth-sankaran]

I offer to work on this for a comprehensive fix for the 4.35 M1 milestone
I already see @gayanper 's thumbs-up, so I hope his efforts in #2265 will not be wasted (once again sorry for my slowness in reviewing).

I am sorry I missed all the context and jumped right in assigning to myself. It was not proper. I'll extricate myself from this task - I am terribly overbooked anyway.

Sorry about the confusion gentlemen.

[gayanper]

@srikanth-sankaran please continue with this task since i will not be able to work on ny PR for a while since I will have to focus on some other languages. But i will try to fine time get back on contributing in few months.

[srikanth-sankaran]

@srikanth-sankaran please continue with this task since i will not be able to work on ny PR for a while since I will have to focus on some other languages. But i will try to fine time get back on contributing in few months.

Thanks @gayanper - truth be told, I jumped into it rather impulsively - I am overbooked, terribly so for the next few months. Let whichever one of us can spare time sooner pick it up - OK ? :)

[srikanth-sankaran]

@srikanth-sankaran please continue with this task since i will not be able to work on ny PR for a while since I will have to focus on some other languages. But i will try to fine time get back on contributing in few months.

Thanks @gayanper - truth be told, I jumped into it rather impulsively - I am overbooked, terribly so for the next few months. Let whichever one of us can spare time sooner pick it up - OK ? :)

(But all that said, given there is already a PR - which I hadn't noticed when I self-assigned - I just saw the ticket didn't have a owner AND there was no PR linked to it saying "may be fixed by #xxxx" - I assigned it to myself.

The better plan is to stick to the current plan of action and have @stephan-herrmann review it and help you take it forward and wrap it up)

[stephan-herrmann]

As opposed to the admittedly-poorly-documented-and-hacky-and-cobbled-together-in-the-last-minute fallbackToSpringForward - that no doubt qualifies as black magic. And one that caused actual grief.

I was thinking of exactly that area of code.

My comment was given from quite some distance as currently I don't have the time to check if there is any real connection between completion for switch and lambda, parser-wise. At least they share the token ->.

[srikanth-sankaran]

As opposed to the admittedly-poorly-documented-and-hacky-and-cobbled-together-in-the-last-minute fallbackToSpringForward - that no doubt qualifies as black magic. And one that caused actual grief.

I was thinking of exactly that area of code.

OK. No objections to that piece being complained about. I will team up with you in dissing it 😉 I will presently not go into why that was even invented in the first place: there were reasons but they can stay out of discussion.

More importantly, the tricks in the grammar have zero connection to fallbackToSpringForward. They did not call for it, necessitate it or require it.

The grammar tricks are highly localized, documented well (enough), can be explained to some one in a few minutes. That work also created some assets (Vanguard parser + a bunch of methods that allow us query the parse without side effects) that have come in handy in other situations as the complexity of Java is increasing.

In fact , I realize now that intuitively I have been according #1045 lower (not low) priority than many other things for those reasons (are highly localized ...)

My comment was given from quite some distance as currently I don't have the time to check if there is any real connection between completion for switch and lambda, parser-wise. At least they share the token ->.

No, they don't!

A recent commit (b294075) carries this checkin comment: (this implements a whole slew of improvements, you may read through the full checkin comment if interested)

*Replace the pair of tokens, first the synthetic BeginCaseExpr followed by '->' in switch rules with a single synthetic CaseArrow. The scanner will disambiguate whether a token '->' is TokenNameARROW used in lambdas or is a CaseArrow used in switch rules by checking if the automaton will shift CaseArrow or not.
*DiagnoseParser's repair of a misclassified '->' will allow it to recover perfectly! Just suppressing the message will do.

The latter point may need explanation.

This is how the scanner handles a '->' now:

if (token == TokenNameARROW) {
	if (this.lookBack[1] == TokenNamedefault)
		return TokenNameCaseArrow;
	if (this.sourceLevel < ClassFileConstants.JDK14 || this.activeParser == null || !this.activeParser.automatonWillShift(TokenNameCaseArrow))
		return TokenNameARROW;
	return TokenNameCaseArrow;
}

So for Switch rules the parser will not see '->' but a new unambiguous token TokenNameCaseArrow

The Scanner cannot ask DiagnoseParser via org.eclipse.jdt.internal.compiler.parser.ConflictedParser.automatonWillShift(int) whether it will shift the token CaseArrow and expect an appropriate answer (multiple state machines, tokens are prebuffered) so the scanner will end up returning '->' instead of CaseArrow.

But I am saying diagnose parse will repair this automatically - we just have to suppress the token '->' misplaced, CaseArrow expected style of diagnostic.

[srikanth-sankaran]

My comment was given from quite some distance as currently I don't have the time to check if there is any real connection between completion for switch and lambda, parser-wise. At least they share the token ->.

My previous comment just addressed the (no longer) shared token part.

There is no connection whatsoever between completion for switches and for lambdas.

I only vaguely recall the complications in completing inside lambdas: something about we needed the full body scanned and parsed (IIRC, result expressions influence the poly typing into a concrete typing), some of that code may exist or not etc. There was a tension between completion scanner declaring EOF at completion point and our wanting to continue the parse.

Well before lambdas, anonymous classes had the same problem and there were many many bugs in code selection and completion inside anonymous classes is my memory.

[stephan-herrmann]

As opposed to the admittedly-poorly-documented-and-hacky-and-cobbled-together-in-the-last-minute fallbackToSpringForward - that no doubt qualifies as black magic. And one that caused actual grief.

I was thinking of exactly that area of code.

OK. No objections to that piece being complained about. I will team up with you in dissing it 😉 I will presently not go into why that was even invented in the first place: there were reasons but they can stay out of discussion.

I'm not blaming you for it, but when we want to improve, a shared understanding of its Raison d'Être would be crucial, no?

More importantly, the tricks in the grammar have zero connection to fallbackToSpringForward. They did not call for it, necessitate it or require it.

The grammar tricks are highly localized, documented well (enough), can be explained to some one in a few minutes.

I take your word, and you may find I already started:

• https://github.com/eclipse-jdt/eclipse.jdt.core/wiki/ECJ-Parse
• https://github.com/eclipse-jdt/eclipse.jdt.core/wiki/ECJ-Completion

Will you help me to keep those pages up-to-date and fill in some gaps?

[srikanth-sankaran]

As opposed to the admittedly-poorly-documented-and-hacky-and-cobbled-together-in-the-last-minute fallbackToSpringForward - that no doubt qualifies as black magic. And one that caused actual grief.

I was thinking of exactly that area of code.

OK. No objections to that piece being complained about. I will team up with you in dissing it 😉 I will presently not go into why that was even invented in the first place: there were reasons but they can stay out of discussion.

I'm not blaming you for it, but when we want to improve, a shared understanding of its Raison d'Être would be crucial, no?

Yes. At some point, when I have the time I can take a fresh look at the whole lambda completion thing. I need to reconstruct many things for my own understanding to offer a precise statement of purpose.

More importantly, the tricks in the grammar have zero connection to fallbackToSpringForward. They did not call for it, necessitate it or require it.

The grammar tricks are highly localized, documented well (enough), can be explained to some one in a few minutes.

I take your word,

You don't have to! I don't think there is evidence to the contrary on record. Of these tricks having caused grief.

and you may find I already started:

• https://github.com/eclipse-jdt/eclipse.jdt.core/wiki/ECJ-Parse

• https://github.com/eclipse-jdt/eclipse.jdt.core/wiki/ECJ-Completion

Will you help me to keep those pages up-to-date and fill in some gaps?

This is yeoman service you have rendered. If such existed when I started out I would have greatly benefitted. And generations to come will value these design notes. Thanks.

Yes, I'll help with this.