Comparator Errors in 4.32 I-Build: I20240324-1800 #1940
Comments
|
I could not find a change in the code referenced in that could cause the difference, so I wonder if this is due to a change in the Java-compiler? |
|
4.32 I-Build: I20240326-1800 - BUILD FAILED : seems infra issue , https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/4475 |
|
This seem to be a side effect of eclipse-jdt/eclipse.jdt.core#1181. @srikanth-sankaran: could you please check if the synthetic method arguments order change was intended for synthetic methods generated for lambdas? |
I'll take a look, Thanks. @stephan-herrmann I may request for help if inference has a role. The change in the PR itself is to not shut the door, I can't see yet how that may result in a class file change. |
It is not actually attributes that got reordered - but field declarations. |
Could fields be ordered by name so they always have a consistent order regardless of position. |
|
Analysis of differences in On master, first we discover the need for a synthetic field to hold the captured outer local And then discover the need for a synthetic field to hold the enclosing instance for But before eclipse-jdt/eclipse.jdt.core#1181 : AND THEN the need for a synthetic field to hold the captured outer local So, in the post eclipse-jdt/eclipse.jdt.core#1181 world, because In the pre eclipse-jdt/eclipse.jdt.core#1181 world, I could confirm that So this change is acceptable, explainable, expected and harmless. Moving on to analyze the next diff in Utilities.class ... Stay tuned |
Can it maybe still considered to have a stable order independent of "discovery order"? e.g. sort all fields by their string name before emitting it to the class byte code? |
|
Utilies class comparator error analysis: This is very similar to the above. On master, we first discover the captured outer local and in the pre eclipse-jdt/eclipse.jdt.core#1181 world, I could confirm that LambdaExpression.analyzeExceptions() is short circuited due to copy.hasIgnoredMandatoryErrors being true - so both are discovered in Analyze phase in the reversed order. So this change is acceptable, explainable, expected and harmless. |
I can assure you the change was not intended as I don't want to cause or deal with comparator errors 😆 But the differences are acceptable, explainable, expected and harmless. |
Basically we were short circuiting some code in the compiler viz This batch of diffs is harmless. |
While that is true, we are aware that by not short circuiting |
Not sure. Doing that is much more work upfront than having to deal with cases like this. I am also not sure what JVMS demands for declared fields. (Here we are dealing with synthetics.) |
|
(BTW, this is a great example of a scenario where a compiler writer wouldn't think of looking into JDT build log themselves for a comparator errors - I was making a change far removed from code generation - or so I thought :) So, there is value in having comparator errors reported one of the "checks" and have them exposed very visibly.) |
I don't think compilers can reorder fields in general - this will affect initialization sequence and exceptions raised etc. Here we are dealing with compiler inserted synthetics - so reordering is not an issue |
|
Thank you @srikanth-sankaran for checking.
I think so as well. Of comparator-errors in the I-build can cause some tedious work when qualifiers need to be bumped, but it I don't think it is dramatic. |
|
I agree that we don't have a problem here. Still, we have the option to sort synthetic fields, since no spec defines their order. Had we done so in the past then eclipse-jdt/eclipse.jdt.core#1181 would not have created any bytecode difference. @srikanth-sankaran , do we expect more such accidental changes of order, that would benefit from introducing ordering now? Or are we fine with the (small) risk of having to perform similar analysis again after future compiler changes cause similar bytecode differences? |
Even with synthetics we don't have complete control over order. For example anonymous class constructors must emit enclosing instances ahead of captured locals. This is because the code invoking the constructor may be emitted by a different compiler than the one that emitted the constructor body. Such a case doesn't arise with lambdas. So this may end up being a special case where we have a little bit freedom. I will leave the sleeping dog lie 😊 |
Since https://download.eclipse.org/eclipse/downloads/drops4/I20240324-1800/ there are two unanticipated comparator errors:
While the first one is already resolved by a change in the affected bundle the second one still requires a enforced qualifier update to resolve. While it is easy to fix, I wonder if this is expected or not?
The text was updated successfully, but these errors were encountered: