Create variable code action is not supported at the top level

[nadeeshaan]

Description: Consider the following source snippet, where the create variable code action should be suggested. Due to the parser does not recover the statement in a way where a variable assignment required can be suggested, we cannot provide the create variable code action.

Steps to reproduce:

import ballerina/module1;

new module1:Listener(12);

Affected Versions: SLP5 at least

[lochana-chathura]

For above code segment, 1). Expected recovery

MISSING[] MISSING[] MISSING[=]new module1:Listener(12);

2). Actual recovery

MISSING[@] INVALID[new] module1:Listener
 (12) MISSING[];

From the error handler's point of view, error handler sees both paths. First path has 3 INSERTS whereas second path has only 1 INSERT and 1 REMOVE. Therefore, it picks the latter path as the optimal path.

From parser's point of view, it cannot identify the missing assignment without parsing the entire new module1:Listener(12) block.

[lochana-chathura]

One way we can try to solve this problem is to allow parsing rhs of the top level declarations also at top level and then log an error. Need to consider the effect to other types of recoveries at top level with such change.

[SupunS]

The current parser is a 'predictive top-down parser', which means it determines what to parse (figures out whats the parent node is), and then parses its children, which are supposed to be there.

But it looks to me as if this use-case requires a 'bottom-up parsing'. That is, first parse the members (expr in this case), and then depending on the members, we determine what should be the parent node. (For the local vars scenario, this is not the case. There are some expressions/actions that are anyway allowed at the statement level. So the parsing is still top-down, and we have only added some validation on top of it)

I'm not sure trying to have both of these in a single parser is a good idea - may leave to some unpredictable parsing results for more prominent use-cases. (e.g: A func without the name now suddenly becomes an expression, and has a chance to become a module-var-decl. Becomes even harder to decide what we should do). We can try of-course. But maybe we are better off saying that we can't/won't handle all the junk code users write.

Just sharing my 2-cents :)

[lochana-chathura]

The current parser is a 'predictive top-down parser', which means it determines what to parse (figures out whats the parent node is), and then parses its children, which are supposed to be there.

But it looks to me as if this use-case requires a 'bottom-up parsing'. That is, first parse the members (expr in this case), and then depending on the members, we determine what should be the parent node. (For the local vars scenario, this is not the case. There are some expressions/actions that are anyway allowed at the statement level. So the parsing is still top-down, and we have only added some validation on top of it)

I'm not sure trying to have both of these in a single parser is a good idea - may leave to some unpredictable parsing results for more prominent use-cases. (e.g: A func without the name now suddenly becomes an expression, and has a chance to become a module-var-decl. Becomes even harder to decide what we should do). We can try of-course. But maybe we are better off saying that we can't/won't handle all the junk code users write.

Just sharing my 2-cents :)

Awesome. thanks for the input. :)

[rasika]

@lochana-chathura can we prioritize this issue for the post-alpha?

[lochana-chathura]

Agreed to not include in Swan Lake.

[lochana-chathura]

Add the following line segment at the class level of a java file. System.lineSeparator();

If you try to see the code actions/ tips in intelliIdea, it does not show create variable. In our ballerina use case, we have the same issue for create Variable code action where the parser does not give the variable assignment required diagnostic. I think this problem is a universal one😀

Also, look at the provided diagnostics in such scenarios. ex2: testMethod(null, null);

Nadeeshaan Gunasinghe, Mar 24, 11:11 PM

[IMS94]

@lochana-chathura what if we we give a higher score to insertions than removals? Assuming the chance of a user removing something he/she wrote is low compared to adding new code?

[IMS94]

Closing this since it has been open for too long. And there's no straightforward way for the parser to address this scenario. Also as mentioned in a comment, intelliJ IDEA have behaves the same. Please create a new issue if this feature is needed.