Bugzilla – Full Text Bug Listing
| Summary: | [12] Switch AST to JLS12 | ||
|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Sarika Sinha <sarika.sinha> |
| Component: | UI | Assignee: | Noopur Gupta <noopur_gupta> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | daniel_megert, jarthana, kalyan_prasad, pradeepb |
| Version: | 4.11 | ||
| Target Milestone: | BETA J12 | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: |
https://git.eclipse.org/r/138199 https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=3ccfaaed9e85323fc58c3622149cfd4691028aca https://git.eclipse.org/r/138522 https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=d741b9adc96120ec61af55023608728c3adcba21 https://git.eclipse.org/r/139087 https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=a9867813623f0dd0dfe62452af0a5a6cd11fc147 |
||
| Whiteboard: | |||
| Bug Depends on: | 542558, 543720, 544748, 545497, 545539 | ||
| Bug Blocks: | 543798, 543831, 544817, 545120 | ||
|
Description
Sarika Sinha
This may need touching some bundles whenever IASTSharedValues.SHARED_AST_LEVEL is changed. See bug 539509 comment #7. I'll wait for bug 542558 comment# 31 to be fixed before continuing here to avoid the rework (see bug 499336 comment# 6). (In reply to Noopur Gupta from comment #3) > I'll wait for bug 542558 comment# 31 to be fixed before continuing here to > avoid the rework (see bug 499336 comment# 6). We will have to revisit this policy or find a way out for e.g, working with stubs and such, to be able to cope with the new Java release cycles. A new Java release every few months gives us very little time to do things in sequence and just won't work when there are many new language features. That said, I believe this is unblocked now? (In reply to Jay Arthanareeswaran from comment #4) > We will have to revisit this policy or find a way out for e.g, working with > stubs and such, to be able to cope with the new Java release cycles. A new > Java release every few months gives us very little time to do things in > sequence and just won't work when there are many new language features. The same was discussed yesterday in the team meeting. Please provide any inputs during the next meeting. > That said, I believe this is unblocked now? Yes, though some polishing is still pending in the AST. I am already working on this bug and it should be released before end of this week. (In reply to Noopur Gupta from comment #5) > (In reply to Jay Arthanareeswaran from comment #4) > > We will have to revisit this policy or find a way out for e.g, working with > > stubs and such, to be able to cope with the new Java release cycles. A new > > Java release every few months gives us very little time to do things in > > sequence and just won't work when there are many new language features. > The same was discussed yesterday in the team meeting. Please provide any > inputs during the next meeting. I was merely recording whatever little was said in the meeting. I will try to come up with an approach or a feasibility study for the next cycle. (In reply to Jay Arthanareeswaran from comment #6) > I will try to come up with an approach or a feasibility study > for the next cycle. Thanks. It will be good to remove this dependency if feasible or modify the availability expectations for such dependent tasks from the next cycle. I have released the changes (https://git.eclipse.org/r/#/c/138199/) with: https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?h=BETA_JAVA_12&id=3ccfaaed9e85323fc58c3622149cfd4691028aca Genie seems to be having some problems in adding comments/links for new Gerrit patches (I noticed many new patches with missing Genie updates in JDT UI today). (In reply to Noopur Gupta from comment #1) > This may need touching some bundles whenever > IASTSharedValues.SHARED_AST_LEVEL is changed. See bug 539509 comment #7. This maybe required after merging to master and will be known after the test I-build. New Gerrit change created: https://git.eclipse.org/r/138199 Gerrit change https://git.eclipse.org/r/138199 was merged to [BETA_JAVA_12]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=3ccfaaed9e85323fc58c3622149cfd4691028aca See bug 545194 also. Those fixes should have gone in along with this bug. Reopening to make changes in UI code after AST Rewrite fix (bug 543720). New Gerrit change created: https://git.eclipse.org/r/138522 Gerrit change https://git.eclipse.org/r/138522 was merged to [BETA_JAVA_12]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=d741b9adc96120ec61af55023608728c3adcba21 Please add a comment here if further changes/additions are done in the AST APIs or NaiveASTFlattener for Java 12. (In reply to Noopur Gupta from comment #14) > Please add a comment here if further changes/additions are done in the AST > APIs or NaiveASTFlattener for Java 12. Some changes are expected for BreakStatement via Bug 544748 (In reply to Sarika Sinha from comment #15) > (In reply to Noopur Gupta from comment #14) > > Please add a comment here if further changes/additions are done in the AST > > APIs or NaiveASTFlattener for Java 12. > > Some changes are expected for BreakStatement via Bug 544748 Changes have been released. (In reply to Sarika Sinha from comment #16) > (In reply to Sarika Sinha from comment #15) > > (In reply to Noopur Gupta from comment #14) > > > Please add a comment here if further changes/additions are done in the AST > > > APIs or NaiveASTFlattener for Java 12. > > > > Some changes are expected for BreakStatement via Bug 544748 > > Changes have been released. Reopening to check if UI code needs changes based on the above. Via 542558, SWITCH_LABELED_RULE_PROPERTY boolean property has been added to SwitchCase, it might be required if ":" changes to "->" or vice versa . (In reply to Sarika Sinha from comment #18) > Via 542558, SWITCH_LABELED_RULE_PROPERTY boolean property has been added to > SwitchCase, it might be required if ":" changes to "->" or vice versa . Need to evaluate if the new property requires any updates in UI code. (In reply to Noopur Gupta from comment #19) > (In reply to Sarika Sinha from comment #18) > > Via 542558, SWITCH_LABELED_RULE_PROPERTY boolean property has been added to > > SwitchCase, it might be required if ":" changes to "->" or vice versa . > > Need to evaluate if the new property requires any updates in UI code. No changes required for SWITCH_LABELED_RULE_PROPERTY. Checking changes released in bug 544748... (In reply to Noopur Gupta from comment #20) > Checking changes released in bug 544748... The new API and the fix in bug 544748 are still not clear to me. Consider this example: public static void foo1(int num) { int res= switch (num) { case 100-> 1; case 200-> 2; case 300,3000-> 3; default -> 6; }; } After the commit in bug 544748, I don't see anything on hover while debugging this switch expression after '->' and nothing is shown on hovering the BreakStatement node. Why is the non-implicit user code ('1;', '2;' etc.) also not shown while debugging? Looking at the comments in that bug, I understand that the guideline is to ignore the BreakStatement node when the API returns true. But in this example, it also contains the user entered code as its child, so ignoring the node will not be correct. Also, consider your request in bug 545249 as I think that will also have the same issue while using this API, but Kalyan can check and comment on that bug. Please explain if I am missing something here. This bug is a blocker for the BETA release. (In reply to Noopur Gupta from comment #21) > (In reply to Noopur Gupta from comment #20) > > Checking changes released in bug 544748... > > The new API and the fix in bug 544748 are still not clear to me. > > Consider this example: > public static void foo1(int num) { > int res= switch (num) { > case 100-> 1; > case 200-> 2; > case 300,3000-> 3; > default -> 6; > }; > } > > After the commit in bug 544748, I don't see anything on hover while > debugging this switch expression after '->' and nothing is shown on hovering > the BreakStatement node. Why is the non-implicit user code ('1;', '2;' etc.) > also not shown while debugging? Yes, this is being tracked by Bug 545497, I am working on it. > > Looking at the comments in that bug, I understand that the guideline is to > ignore the BreakStatement node when the API returns true. But in this > example, it also contains the user entered code as its child, so ignoring > the node will not be correct. Also, consider your request in bug 545249 as I > think that will also have the same issue while using this API, but Kalyan > can check and comment on that bug. > > Please explain if I am missing something here. This bug is a blocker for the > BETA release. Bug 545249 - If implicit break does not have a label or optional expression then we need not show in the AST view, if it has additional label or expression node it should be shown. (In reply to Sarika Sinha from comment #22) > Yes, this is being tracked by Bug 545497, I am working on it. Difficult to find the bug without any dependencies added to it. > Bug 545249 - If implicit break does not have a label or optional expression > then we need not show in the AST view, if it has additional label or > expression node it should be shown. Kalyan, please check if this is fine for all the scenarios possible in bug 545249. Moving this bug to 4.12 as I need more time to explore this new API. Bug 545497 has been resolved in BETA Java 12 branch. I see the API BreakStatement.setImplicit(boolean isImplicit) is also added. Based on bug 544748 comment #5, this is implicit to the compiler and need not be set/unset explicitly. So, is it required to be set by clients or not via the API #setImplicit? (In reply to Noopur Gupta from comment #25) > I see the API BreakStatement.setImplicit(boolean isImplicit) is also added. > > Based on bug 544748 comment #5, this is implicit to the compiler and need > not be set/unset explicitly. > > So, is it required to be set by clients or not via the API #setImplicit? I think if it needs to be set by clients while creating the BreakStatement node then it should also have a corresponding property. (Though, setting by clients will be contradictory to its name.) If not required to be set from outside, then BreakStatement.setImplicit(boolean isImplicit) API is not required and should be removed. (In reply to Noopur Gupta from comment #26) > > I think if it needs to be set by clients while creating the BreakStatement > node then it should also have a corresponding property. (Though, setting by > clients will be contradictory to its name.) > > If not required to be set from outside, then > BreakStatement.setImplicit(boolean isImplicit) API is not required and > should be removed. Our thinking was that clients should never be required to set isImplicit Flag as it is generated by Compiler. (In reply to Sarika Sinha from comment #27) > (In reply to Noopur Gupta from comment #26) > > > > I think if it needs to be set by clients while creating the BreakStatement > > node then it should also have a corresponding property. (Though, setting by > > clients will be contradictory to its name.) > > > > If not required to be set from outside, then > > BreakStatement.setImplicit(boolean isImplicit) API is not required and > > should be removed. > > Our thinking was that clients should never be required to set isImplicit > Flag as it is generated by Compiler. If setting this flag is handled internally in JDT Core on node creation (not sure where it needs to be handled, in AST creation, AST Rewrite or compiler), then clients don't need it. So, can the extra #set API be removed to avoid the confusion and set clear expectations? (In reply to Noopur Gupta from comment #28) > > If setting this flag is handled internally in JDT Core on node creation (not > sure where it needs to be handled, in AST creation, AST Rewrite or > compiler), then clients don't need it. So, can the extra #set API be removed > to avoid the confusion and set clear expectations? We need it in ASTConverter and test plugin, so removing API may not be feasible. We can add some extra information in Javadoc for the API to avoid confusion for clients. (In reply to Sarika Sinha from comment #29) > (In reply to Noopur Gupta from comment #28) > > > > If setting this flag is handled internally in JDT Core on node creation (not > > sure where it needs to be handled, in AST creation, AST Rewrite or > > compiler), then clients don't need it. So, can the extra #set API be removed > > to avoid the confusion and set clear expectations? > > We need it in ASTConverter and test plugin, so removing API may not be > feasible. We can add some extra information in Javadoc for the API to avoid > confusion for clients. Adding an API for a test plug-in is strange. For ASTConverter, will it not work if it's made package accessible? Also, updating Javadoc along with that will be good. (In reply to Noopur Gupta from comment #30) > Adding an API for a test plug-in is strange. For ASTConverter, will it not > work if it's made package accessible? Also, updating Javadoc along with that > will be good. We had a quick discussion, we actually should not use it in test also as client is not supposed to use it. I will be working on it and will make it package scoped. Can we close this bug targeting to BETA_JAVA_12 branch ? And create a followup bug for remaining work for 4.12 ? We can not close the Root bug and Infrastructure bug if this remains open. (In reply to Sarika Sinha from comment #32) > Can we close this bug targeting to BETA_JAVA_12 branch ? > And create a followup bug for remaining work for 4.12 ? > We can not close the Root bug and Infrastructure bug if this remains open. I would like to keep AST related issues in a single bug for future reference instead of spreading it across multiple bugs. (In reply to Sarika Sinha from comment #31) > (In reply to Noopur Gupta from comment #30) > > > Adding an API for a test plug-in is strange. For ASTConverter, will it not > > work if it's made package accessible? Also, updating Javadoc along with that > > will be good. > > We had a quick discussion, we actually should not use it in test also as > client is not supposed to use it. I will be working on it and will make it > package scoped. Bug 545539. New Gerrit change created: https://git.eclipse.org/r/139087 Gerrit change https://git.eclipse.org/r/139087 was merged to [BETA_JAVA_12]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=a9867813623f0dd0dfe62452af0a5a6cd11fc147 (In reply to Eclipse Genie from comment #36) > Gerrit change https://git.eclipse.org/r/139087 was merged to [BETA_JAVA_12]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=a9867813623f0dd0dfe62452af0a5a6cd11fc147 Released the fix in BETA branch. It should get into master with the merge that's still pending. |