Fix compile error with final field assignment in constructor with prologue in local class in constructor

[coehlrich]

What it does

Fixes #4700

How to test

Attempt to compile

public class Test {
	private final Object a;
	public Test() {
		a = new Object();
		class Test2 {
			private final Object b;
			public Test2() {
				System.out.println();
				super();
				b = new Object();
			}
		}
	}
}

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[iloveeclipse]

Two previous Jenkins build silently did nothing...

I've manually triggered Jenkins build https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-4701/3/console

If the problem persists, please file a ticket at https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/ or ping me here.

[stephan-herrmann]

Cool, this effectively fixes the issue, but something smells strangely here: the flow info that produced the bogus error concerned the field a of the outer class, i.e., we were actually misinterpreting the field index 0.

@coehlrich do you think we can streamline this code section wrt field of the enclosing class? See this comment few lines above:

// discards info about fields of inclosing classes

Given that no analysis within the current type should bother about fields of the enclosing class (?), could we find a single location for calling unconditionalFieldLessCopy() once and for all?

[coehlrich]

Currently internalAnalyseCode uses flowInfo without calling unconditionalFieldLessCopy in 5 locations:

• 2 times as arguments for InitializationFlowContext which only uses it in getInitsForFinalBlankInitializationCheck which also looks for the correct parent before returning the corresponding FlowInfo
• 2 times for calls to reachMode which is copied when calling unconditionalFieldLessCopy
• 1 time as an argument when calling analyseCode on all methods that aren't for initialization

None of the regression tests failed when adding flowInfo = flowInfo.unconditionalFieldLessCopy() to the top of internalAnalyseCode

While looking into this I did also notice that constructors with prologues aren't checked to see if all of the blank final fields have been initialized

[coehlrich]

all uses of the methods relating to fields in FlowInfo in ast either only look at fields in the same class or have a check for if the current class is the same as the one containing the field with some also only calling it on the output of getInitsForFinalBlankInitializationCheck. for subclasses of FlowContext I couldn't get it to recognize final field assignments for the outer class in finally blocks or in the condition of a while loop (which still did give a compile error).

[stephan-herrmann]

Code change looks good to me ...

for subclasses of FlowContext I couldn't get it to recognize final field assignments for the outer class in finally blocks or in the condition of a while loop (which still did give a compile error).

Are you saying the fix still isn't complete? If so can you give an example?

Otherwise please explain the implications of your observation :)

Also by merging #4721 I created a conflict in the test class. Please resolve it, thanks.

[stephan-herrmann]

@coehlrich did you see my latest comment above?

[stephan-herrmann]

For FinallyFlowContext and LoopingFlowContext those classes only check bindings from recordFinalAssignment which is called by recordSettingFinal which is only being called right before FlowInfo.markAsDefinitelyAssigned if FlowInfo.isPotentiallyAssigned returns false.

What is the impact of this observation? Do you see a bug?

if FlowInfo.isPotentiallyAssigned() returns true at the point before an assignment, we already report an error. What else do we need to pay attention to?

[stephan-herrmann]

@coehlrich how can we bring this discussion to a conclusion? I still haven't understood if you still see a problem.

[coehlrich]

recordSettingFinal is only called with a field in cases where allowBlankFinalFieldAssigned returns true which checks if the current class is the same class as the one that directly contains the field so FinallyFlowContext and LoopingFlowerContext can't check fields from an enclosing class.

[stephan-herrmann]

Does this observation allow you to create a failing test?

[stephan-herrmann]

recordSettingFinal is only called with a field in cases where allowBlankFinalFieldAssigned returns true which checks if the current class is the same class as the one that directly contains the field so FinallyFlowContext and LoopingFlowerContext can't check fields from an enclosing class.

I finally found the time to check myself, if this lets me identify a bug. So for the case of fields from an enclosing class allowBlankFinalFieldAssigned() returns false, correct. This implies that in such cases we'll never invoke flowContext.recordSettingFinal() regardless of the type of flow context.

But the above false directly catapults us into the else branch reading like this:

			} else {
				currentScope.problemReporter().cannotAssignToFinalField(fieldBinding, this);
			}

So this case is reported as an error without further consultation of flow info nor flow context.

Adding your remark about getInitsForFinalBlankInitializationCheck() we seem to be in the business of reporting missing initialization? But in those cases we are always below a check needBlankFinalFieldInitializationCheck() to the effect that we only ever complain about missing assignments for fields of the current class, which indeed is the right thing to do, as assignment-to-final is the sole responsibility of the class declaring the field.

So, thanks for checking left and right regarding the handling of fields from enclosing classes. If you still see a problem please speak up. Otherwise I will just refresh the branch, add one more test and try to get it merged before tomorrow's M3.

[coehlrich]

Jenkins had another issue with building this PR. For a while after the builds failed the https://ci.eclipse.org/jdt/ was timing out while https://ci.eclipse.org was fine.

[stephan-herrmann]

Thanks, @coehlrich !