Community
Participate
Working Groups
BETA_JAVA7 Improve AST recovery for TryStatement-with-resources: package p; import java.io.FileInputStream; public class ResourcesRecovery { void foo(String name) { try (new FileInputStream(name)) { } try (FileInputStream fis= new FileInputStream(name)) { } } void bar(String name) { if (true) { try (new FileInputStream(name)) { } System.out.println("Hi"); } } } In foo(), I currently get a recovered WhileStatement, but I'd expect a TryStatement with a recovered resource (VariableDeclarationExpression with a dummy type and a fragment with a dummy name). In bar(), not even the IfStatement is recovered.
I tried to omit the resource specification here and found that even the simple try without resources does not get recovered. Some grammar change has caused the recovery of try statements to break in BETA_JAVA7 branch. Will have to investigate more.
The RecoveryScanner here returns a 'while' token instead of 'try' token while parsing. Might be a bug in the recovery scanner.
(In reply to comment #2) > The RecoveryScanner here returns a 'while' token instead of 'try' token while > parsing. Might be a bug in the recovery scanner. I was wrong. The DiagnoseParser has set while as the replacement token in DiagnoseParser#reportPrimaryError(..). Thats why RecoveryScanner returns 'while'.
I think the recovery currently obtained is the best bet and is also the most correct. This is because, basically to convert try (new FileInputStream(name)) into a valid syntax of a try with resources statement, it will take a lot more additional tokens and to convert it into a normal try statement will involve deletion of many tokens. Compared to that, substituting try with while here seems the "shortest possible" route of getting a valid syntax. This is vindicated by the observations that 1) Parser gives us a recovered for statement when i write try(new FileInputStream(name);;), which seems quite intuitive too. 2) It gives us a recovered try statement when i write try(new FileInputStream(name);), since in only the try with resources do we use a single semicolon inside the (..).
I don't think the main guide to judge recovery quality should be the amount of changed tokens. From a developer's perspective, the winning recovery should always be one that considers the longest prefix of the CU as correct (which is also the assumption behind code assist). When the "try" keyword is already there, it is very unlikely that I actually meant "while" or "for".
The limitation in case of parser recovery is that we have only raw tokens and some rules to fit them in. There's no clear interpretation or meaning out of them. The parser tries to do 4 things to recover from an error - deletion of an erreneous token, substitution of another token, insertion of some token and merge of two tokens. In case more than one strategy succeeds, it relies on the "distance" (the number of tokens that can be parsed successfully if one strategy is applied) to decide which one wins. So, in the given case, clearly deletion (of the '(' after try and 'new' and so on)succeeds, but so does substitution of try with while. But the latter succeeds with a greater value of distance. All this happens in DiagnoseParser.checkPrimaryDistance(..). While i'm not really giving up on this, the more I investigate, the more I feel that there's nothing really wrong with the error recovery here.
Maybe you can talk the recovery into preferring matching prefixes by adding a weight to changed tokens (e.g. count substitution of "try" as two or even more changes)
Srikanth, can you also take a look at this when you have time and see if a non-hackish fix is possible. Thanks!
Reassigning to Srikanth. When the code is completely busted, it is difficult to recover the way you want it. Even if it looks intuitive for the user, our recovery mechanism has known limitations. Changing it to fix this case might make it more fragile in other situations.
We discussed this and there is some nervousness around fixing this one for the 3.7.1 timeframe. Touching the statement recovery is not something we should do on the last minute - this unfortunately being an area that is poorly documented and has proved to black art to some degree. Retargetted to 3.8.
Don't expect to get to this anytime soon. Will consider for next release.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
