"Workspace options should be back to their default" - reproducer

[stephan-herrmann]

The infamous test failure "Workspace options should be back to their default" can be reproduced by running AllJavaModelTests with everything prior to JavaCorePreferenceModifyListenerTest disabled / commented. The sequence of JavaCorePreferenceModifyListenerTest + NullAnnotationModelTests creates the following failure (interestingly only once):

junit.framework.ComparisonFailure: Workspace options should be back to their default.
----------- Expected ------------
CompilerOptions:\n
    - local variables debug attributes: ON\n
    - line number debug attributes: ON\n
    - source debug attributes: ON\n
    - MethodParameters attributes: do not generate\n
    - Generic signature for lambda expressions: do not generate\n
    - preserve all local variables: ON\n
    - method with constructor name: warning\n
    - overridden package default method: warning\n
    - deprecation: warning\n
    - removal: warning\n
    - masked catch block: warning\n
    - unused local variable: warning\n
    - unused parameter: ignore\n
    - unused exception parameter: ignore\n
    - unused import: warning\n
    - synthetic access emulation: ignore\n
    - assignment with no effect: warning\n
    - non externalized string: ignore\n
    - static access receiver: warning\n
    - indirect static access: ignore\n
    - incompatible non inherited interface method: warning\n
    - unused private member: warning\n
    - local variable hiding another variable: ignore\n
    - field hiding another variable: ignore\n
    - type hiding another type: warning\n
    - possible accidental boolean assignment: ignore\n
    - superfluous semicolon: ignore\n
    - uncommented empty block: ignore\n
    - unnecessary type check: ignore\n
    - javadoc comment support: ON\n
        + invalid javadoc: ignore\n
        + report invalid javadoc tags: disabled\n
            * deprecated references: disabled\n
            * not visible references: disabled\n
        + visibility level to report invalid javadoc tags: public\n
        + missing javadoc tags: ignore\n
        + visibility level to report missing javadoc tags: public\n
        + report missing javadoc tags for method type parameters: disabled\n
        + report missing javadoc tags in overriding methods: disabled\n
        + missing javadoc comments: ignore\n
        + report missing tag description option: return_tag\n
        + visibility level to report missing javadoc comments: public\n
        + report missing javadoc comments in overriding methods: disabled\n
    - finally block not completing normally: warning\n
    - report unused declared thrown exception: ignore\n
    - report unused declared thrown exception when overriding: disabled\n
    - report unused declared thrown exception include doc comment reference: enabled\n
    - report unused declared thrown exception exempt exception and throwable: enabled\n
    - unnecessary else: ignore\n
    - JDK compliance level: 1.8\n
    - JDK source level: 1.8\n
    - JDK target level: 1.8\n
    - verbose : OFF\n
    - produce reference info : OFF\n
    - parse literal expressions as constants : ON\n
    - encoding : UTF-8\n
    - task tags: TODO,FIXME,XXX\n
    - task priorities : NORMAL,HIGH,NORMAL\n
    - report deprecation inside deprecated code : disabled\n
    - report deprecation when overriding deprecated method : disabled\n
    - report unused parameter when implementing abstract method : disabled\n
    - report unused parameter when overriding concrete method : disabled\n
    - report unused parameter include doc comment reference : enabled\n
    - report constructor/setter parameter hiding existing field : disabled\n
    - inline JSR bytecode : enabled\n
    - share common finally blocks : disabled\n
    - report unavoidable generic type problems : enabled\n
    - unsafe type operation: warning\n
    - unsafe raw type: warning\n
    - final bound for type parameter: warning\n
    - missing serialVersionUID: warning\n
    - varargs argument need cast: warning\n
    - forbidden reference to type with access restriction: error\n
    - discouraged reference to type with access restriction: warning\n
    - null reference: warning\n
    - potential null reference: ignore\n
    - redundant null check: ignore\n
    - autoboxing: ignore\n
    - annotation super interface: warning\n
    - missing @Override annotation: ignore\n
    - missing @Override annotation for interface method implementation: enabled\n
    - missing @Deprecated annotation: ignore\n
    - incomplete enum switch: warning\n
    - raise null related warnings for variables tainted in assert statements: disabled\n
    - suppress warnings: enabled\n
    - suppress optional errors: disabled\n
    - unhandled warning token: warning\n
    - unused warning token: warning\n
    - unused label: warning\n
    - treat optional error as fatal: disabled\n
    - parameter assignment: ignore\n
    - generate class files: enabled\n
    - process annotations: disabled\n
    - unused type arguments for method/constructor invocation: warning\n
    - redundant superinterface: ignore\n
    - comparing identical expr: warning\n
    - missing synchronized on inherited method: ignore\n
    - should implement hashCode() method: ignore\n
    - dead code: warning\n
    - dead code in trivial if statement: disabled\n
    - tasks severity: warning\n
    - unused object allocation: ignore\n
    - method can be static: ignore\n
    - method can be potentially static: ignore\n
    - redundant specification of type arguments: ignore\n
    - resource is not closed: warning\n
    - resource may not be closed: ignore\n
    - resource should be handled by try-with-resources: ignore\n
    - Unused Type Parameter: ignore\n
    - pessimistic null analysis for free type variables: warning\n
    - report unsafe nonnull return from legacy method: warning\n
    - unlikely argument type for collection methods: warning\n
    - unlikely argument type for collection methods, strict check against expected type: disabled\n
    - unlikely argument types for equals(): info\n
    - API leak: warning\n
    - unstable auto module name: warning\n
    - SuppressWarnings not fully analysed: info
------------ but was ------------
CompilerOptions:\n
    - local variables debug attributes: ON\n
    - line number debug attributes: ON\n
    - source debug attributes: ON\n
    - MethodParameters attributes: do not generate\n
    - Generic signature for lambda expressions: do not generate\n
    - preserve all local variables: ON\n
    - method with constructor name: warning\n
    - overridden package default method: warning\n
    - deprecation: warning\n
    - removal: warning\n
    - masked catch block: warning\n
    - unused local variable: warning\n
    - unused parameter: ignore\n
    - unused exception parameter: ignore\n
    - unused import: warning\n
    - synthetic access emulation: ignore\n
    - assignment with no effect: warning\n
    - non externalized string: ignore\n
    - static access receiver: warning\n
    - indirect static access: ignore\n
    - incompatible non inherited interface method: warning\n
    - unused private member: warning\n
    - local variable hiding another variable: ignore\n
    - field hiding another variable: ignore\n
    - type hiding another type: warning\n
    - possible accidental boolean assignment: ignore\n
    - superfluous semicolon: ignore\n
    - uncommented empty block: ignore\n
    - unnecessary type check: ignore\n
    - javadoc comment support: ON\n
        + invalid javadoc: ignore\n
        + report invalid javadoc tags: disabled\n
            * deprecated references: disabled\n
            * not visible references: disabled\n
        + visibility level to report invalid javadoc tags: public\n
        + missing javadoc tags: ignore\n
        + visibility level to report missing javadoc tags: public\n
        + report missing javadoc tags for method type parameters: disabled\n
        + report missing javadoc tags in overriding methods: disabled\n
        + missing javadoc comments: ignore\n
        + report missing tag description option: return_tag\n
        + visibility level to report missing javadoc comments: public\n
        + report missing javadoc comments in overriding methods: disabled\n
    - finally block not completing normally: warning\n
    - report unused declared thrown exception: ignore\n
    - report unused declared thrown exception when overriding: disabled\n
    - report unused declared thrown exception include doc comment reference: enabled\n
    - report unused declared thrown exception exempt exception and throwable: enabled\n
    - unnecessary else: ignore\n
    - JDK compliance level: 11\n
    - JDK source level: 11\n
    - JDK target level: 11\n
    - verbose : OFF\n
    - produce reference info : OFF\n
    - parse literal expressions as constants : ON\n
    - encoding : UTF-8\n
    - task tags: TODO,FIXME,XXX\n
    - task priorities : NORMAL,HIGH,NORMAL\n
    - report deprecation inside deprecated code : disabled\n
    - report deprecation when overriding deprecated method : disabled\n
    - report unused parameter when implementing abstract method : disabled\n
    - report unused parameter when overriding concrete method : disabled\n
    - report unused parameter include doc comment reference : enabled\n
    - report constructor/setter parameter hiding existing field : disabled\n
    - inline JSR bytecode : enabled\n
    - share common finally blocks : disabled\n
    - report unavoidable generic type problems : enabled\n
    - unsafe type operation: warning\n
    - unsafe raw type: warning\n
    - final bound for type parameter: warning\n
    - missing serialVersionUID: warning\n
    - varargs argument need cast: warning\n
    - forbidden reference to type with access restriction: error\n
    - discouraged reference to type with access restriction: warning\n
    - null reference: warning\n
    - potential null reference: ignore\n
    - redundant null check: ignore\n
    - autoboxing: ignore\n
    - annotation super interface: warning\n
    - missing @Override annotation: ignore\n
    - missing @Override annotation for interface method implementation: enabled\n
    - missing @Deprecated annotation: ignore\n
    - incomplete enum switch: warning\n
    - raise null related warnings for variables tainted in assert statements: disabled\n
    - suppress warnings: enabled\n
    - suppress optional errors: disabled\n
    - unhandled warning token: warning\n
    - unused warning token: warning\n
    - unused label: warning\n
    - treat optional error as fatal: disabled\n
    - parameter assignment: ignore\n
    - generate class files: enabled\n
    - process annotations: disabled\n
    - unused type arguments for method/constructor invocation: warning\n
    - redundant superinterface: ignore\n
    - comparing identical expr: warning\n
    - missing synchronized on inherited method: ignore\n
    - should implement hashCode() method: ignore\n
    - dead code: warning\n
    - dead code in trivial if statement: disabled\n
    - tasks severity: warning\n
    - unused object allocation: ignore\n
    - method can be static: ignore\n
    - method can be potentially static: ignore\n
    - redundant specification of type arguments: ignore\n
    - resource is not closed: warning\n
    - resource may not be closed: ignore\n
    - resource should be handled by try-with-resources: ignore\n
    - Unused Type Parameter: ignore\n
    - pessimistic null analysis for free type variables: warning\n
    - report unsafe nonnull return from legacy method: warning\n
    - unlikely argument type for collection methods: warning\n
    - unlikely argument type for collection methods, strict check against expected type: disabled\n
    - unlikely argument types for equals(): info\n
    - API leak: warning\n
    - unstable auto module name: warning\n
    - SuppressWarnings not fully analysed: info
--------- Difference is ----------
 expected:<... compliance level: 1[.8\n
    - JDK source level: 1.8\n
    - JDK target level: 1.8]\n
    - verbose : OFF\...> but was:<... compliance level: 1[1\n
    - JDK source level: 11\n
    - JDK target level: 11]\n
    - verbose : OFF\...>
    at org.eclipse.jdt.core.tests.junit.extension.TestCase.assertStringEquals(TestCase.java:266)
    at org.eclipse.jdt.core.tests.junit.extension.TestCase.assertEquals(TestCase.java:242)
    at org.eclipse.jdt.core.tests.model.AbstractJavaModelTests.tearDown(AbstractJavaModelTests.java:3936)
    at org.eclipse.jdt.core.tests.model.ReconcilerTests.tearDown(ReconcilerTests.java:334)
    at junit.framework.TestCase.runBare(TestCase.java:147)
    at junit.framework.TestResult$1.protect(TestResult.java:122)
    at junit.framework.TestResult.runProtected(TestResult.java:142)
    at junit.framework.TestResult.run(TestResult.java:125)
    at junit.framework.TestCase.run(TestCase.java:130)
    at junit.framework.TestSuite.runTest(TestSuite.java:241)
    at org.eclipse.jdt.core.tests.model.SuiteOfTestCases$Suite.runTest(SuiteOfTestCases.java:114)
    at junit.framework.TestSuite.run(TestSuite.java:236)
    at org.eclipse.jdt.core.tests.model.SuiteOfTestCases$Suite.superRun(SuiteOfTestCases.java:98)
    at org.eclipse.jdt.core.tests.model.SuiteOfTestCases$Suite$1.protect(SuiteOfTestCases.java:86)
    at junit.framework.TestResult.runProtected(TestResult.java:142)
    at org.eclipse.jdt.core.tests.model.SuiteOfTestCases$Suite.run(SuiteOfTestCases.java:95)
    at junit.framework.TestSuite.runTest(TestSuite.java:241)
    at junit.framework.TestSuite.run(TestSuite.java:236)
    at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:128)
    at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:40)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:529)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:756)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:452)
    at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:74)
    at org.eclipse.pde.internal.junit.runtime.CoreTestApplication.start(CoreTestApplication.java:28)
    at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:136)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:401)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:659)
    at org.eclipse.equinox.launcher.Main.basicRun(Main.java:596)
    at org.eclipse.equinox.launcher.Main.run(Main.java:1467)
    at org.eclipse.equinox.launcher.Main.main(Main.java:1440)

[stephan-herrmann]

See also recent activity in #42 and history in https://bugs.eclipse.org/302850 and https://bugs.eclipse.org/482991

[iloveeclipse]

I have another reproducer and know why that happens. The reason is that we have tests, that import/copy/create projects and if the projects contain .settings directory, code goes to org.eclipse.jdt.launching.JavaRuntime.updateCompliance(IVMInstall) that will set compliance settings to these from the currently used JVM via JavaCore.setOptions(). Stack:

at org.eclipse.jdt.launching.JavaRuntime.updateCompliance(JavaRuntime.java:3382)
    at org.eclipse.jdt.launching.JavaRuntime.initializeVMs(JavaRuntime.java:3316)
    at org.eclipse.jdt.launching.JavaRuntime.getVMInstallTypes(JavaRuntime.java:581)
    at org.eclipse.jdt.internal.launching.JREPreferenceModifyListener.preApply(JREPreferenceModifyListener.java:133)
    at org.eclipse.core.internal.preferences.PreferencesService.lambda$6(PreferencesService.java:411)
    at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:75)
    at org.eclipse.core.internal.preferences.PreferencesService.firePreApplyEvent(PreferencesService.java:411)
    at org.eclipse.core.internal.preferences.PreferencesService.applyPreferences(PreferencesService.java:105)
    at org.eclipse.core.internal.resources.ProjectPreferences.read(ProjectPreferences.java:240)
    at org.eclipse.core.internal.resources.ProjectPreferences.updatePreferences(ProjectPreferences.java:361)
    at org.eclipse.core.internal.resources.File.updateMetadataFiles(File.java:508)
    at org.eclipse.core.internal.localstore.CopyVisitor.copyContents(CopyVisitor.java:109)
    at org.eclipse.core.internal.localstore.CopyVisitor.copy(CopyVisitor.java:73)
    at org.eclipse.core.internal.localstore.CopyVisitor.visit(CopyVisitor.java:207)
    at org.eclipse.core.internal.localstore.UnifiedTree.accept(UnifiedTree.java:119)
    at org.eclipse.core.internal.localstore.FileSystemResourceManager.copy(FileSystemResourceManager.java:383)
    at org.eclipse.core.internal.resources.Resource.copy(Resource.java:572)
    at org.eclipse.core.internal.resources.Project.internalCopy(Project.java:721)
    at org.eclipse.core.internal.resources.Project.copy(Project.java:262)
    at org.eclipse.core.internal.resources.Resource.copy(Resource.java:554)
    at org.eclipse.jdt.core.tests.model.JavaSearchMultipleProjectsTests.testMethodOccurences(JavaSearchMultipleProjectsTests.java:388)

To fix that, we should make sure that JavaRuntime.updateCompliance() never changes JavaCore options if not asked :-)

Options were: 1) set/check JVM properties indicating we are in the JDT tests. => will not work for manual test execution, as every launch config will need extra option. 2) set/check a preference in the JavaOptions itself, indicating we are in the JDT tests. => would also work for manual test execution if set in the base test class / common infra code. 3) dynamically find out that we are testing. => no idea what to look for.

I have a patch that works for 2) but I would like to hear opinions how to fix that, if there are other options possible I didn't listed above.

@stephan-herrmann, @jarthana, @SarikaSinha - WDYT?

[jarthana]

I vaguely remember we have something that will tell us that we are running tests. Perhaps in the area of module resolution. Quick search didn't bring up anything. I have a feeling that @stephan-herrmann remembers this. Worst case, I am sure we have some vm attributes that we are setting only in case of tests, for e.g -Dcomplinace which we can use for options 1/3

[SarikaSinha]

If 3) can be done, it will be the preferred way. Where are we planning to add this check? In Preference Modify Listener?

[iloveeclipse]

If 3) can be done, it will be the preferred way.

Sure. I'm currently thinking about check for the TEST_LIB classpath variable, that is defined in org.eclipse.jdt.core.tests.model plugin and so shouldn't exist in "usual" SDK. The check in org.eclipse.jdt.launching.JavaRuntime.updateCompliance(IVMInstall) would be trivial like:

boolean testsRunning = List.of(JavaCore.getClasspathVariableNames()).contains("TEST_LIB"); //$NON-NLS-1$
if (testsRunning) {
    return;
}

Where are we planning to add this check? In Preference Modify Listener?

In org.eclipse.jdt.launching.JavaRuntime.updateCompliance(IVMInstall)

[iloveeclipse]

See PR: https://github.com/eclipse-jdt/eclipse.jdt.debug/pull/458

[SarikaSinha]

I will prefer it handled in initializeVMs where we should set updateCompliance as false for testing condition.

[stephan-herrmann]

Before reading through all your great findings: I seem to recall that in previous editions of this problem we tried to avoid touching any plugins from JDT/Debug during these tests. There might even be tests checking which plugins where started during tests. Anyone seen this recently?

[iloveeclipse]

No, and that's not what the reason here. Here the discovery of project settings by launcher code triggered JVM update / JavaCore options change. The stack is self explaining.

[stephan-herrmann]

No, and that's not what the reason here. Here the discovery of project settings by launcher code triggered JVM update / JavaCore options change. The stack is self explaining.

My reasoning was and is: if plugin jdt.launching is not active, then nothing gets triggered.

[stephan-herrmann]

I have another reproducer and know why that happens. The reason is that we have tests, that import/copy/create projects and if the projects contain .settings directory, code goes to org.eclipse.jdt.launching.JavaRuntime.updateCompliance(IVMInstall) that will set compliance settings to these from the currently used JVM via JavaCore.setOptions(). Stack:

at org.eclipse.jdt.launching.JavaRuntime.updateCompliance(JavaRuntime.java:3382)
  at org.eclipse.jdt.launching.JavaRuntime.initializeVMs(JavaRuntime.java:3316)

See

• https://bugs.eclipse.org/bugs/show_bug.cgi?id=302850#c25
• https://bugs.eclipse.org/bugs/show_bug.cgi?id=482991#c12

Eventually I dropped the idea to control the set of running plugins: https://bugs.eclipse.org/bugs/show_bug.cgi?id=482991#c14

[stephan-herrmann]

after all the hassle we had in this area, let me naively ask if everybody is sure that updating JDT/Core default options like this is a good idea? Which problem does it solve?

[iloveeclipse]

My reasoning was and is: if plugin jdt.launching is not active, then nothing gets triggered.

I haven't checked it now, but from previous discussions I believe to remember it wasn't easy/possible to not activete it.

[iloveeclipse]

after all the hassle we had in this area, let me naively ask if everybody is sure that updating JDT/Core default options like this is a good idea? Which problem does it solve?

I believe the intent is to provide "reasonable" defaults for the user. We usually ship "default" compliance to be lowest possible (1.8 would be now after https://github.com/eclipse-jdt/eclipse.jdt.core/pull/2551) but of course most users would prefer "current" (or better) JVM version to be default target, so the code does exactly that and sets default compliance to "better" JLS.

[stephan-herrmann]

after all the hassle we had in this area, let me naively ask if everybody is sure that updating JDT/Core default options like this is a good idea? Which problem does it solve?

I believe the intent is to provide "reasonable" defaults for the user. We usually ship "default" compliance to be lowest possible (1.8 would be now after #2551) but of course most users would prefer "current" (or better) JVM version to be default target, so the code does exactly that and sets default compliance to "better" JLS.

Thanks, sounds fair. I guess I'm just not the audience wanting to create tons of projects without explicitly setting compliance settings :)