OPTION_JdtDebugCompileMode has surprising effects on imports

[stephan-herrmann]

In https://bugs.eclipse.org/543604 the OPTION_JdtDebugCompileMode was introduced to suppress a specific error when compiling for debug evaluations. Fine. The option is respected in various locations of the code, but only one of them is covered by a test.

While reviewing PR #424 I detected a strange interaction: if you run the test from the PR, but without using its new OPTION_IgnoreUnnamedModuleForSplitPackage, you will see this error:

	import org.xml.sax.SAXParseException;
	       ^^^^^^^^^^^
The package org.xml.sax is accessible from more than one module: <unnamed>, java.xml

Fine, that's what #424 is all about. BUT if you now enable OPTION_JdtDebugCompileMode that error message is changed to

	import org.xml.sax.SAXParseException;
	       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Only a type can be imported. org.xml.sax.SAXParseException resolves to a package

This indicates that the code change from https://bugs.eclipse.org/543604 within CompilationUnitScope.faultInImports() doesn't work as expected.

@trancexpress @iloveeclipse could you shed a light on why those changes in faultInImports() were made? Should we ...

1. revert those, i.e., restrict the effect of OPTION_JdtDebugCompileMode to QualifiedTypeReference.getTypeBinding(Scope), or
2. fix them so that also imports are exempted from split package checking?

Bonus question: what about the changes in ModuleDeclaration.analyseOneDependency(..)?

[iloveeclipse]

@stephan-herrmann, honestly speaking I can't remember details why which line was changed. That was done to fix evaluation in debugger, so once that was "fixed" (one way or another) we were happy.

But in light of #424 changes, shouldn't debugger use this new preference instead? We don't need to have two workarounds for same problem.

[trancexpress]

@trancexpress @iloveeclipse could you shed a light on why those changes in faultInImports() were made? Should we ...

1. revert those, i.e., restrict the effect of `OPTION_JdtDebugCompileMode` to `QualifiedTypeReference.getTypeBinding(Scope)`, or

2. fix them so that also imports are exempted from split package checking?

Bonus question: what about the changes in ModuleDeclaration.analyseOneDependency(..)?

For CompilationUnitScope.faultInImports() I believe I went over the calls to ProblemReporter.conflictingPackagesFromModules(SplitPackageBinding, ModuleBinding, int, int) and added the preference check. I assume I did the change in ModuleDeclarationlikewise (I can't remember, it has been a while). See also my comment on the gerrit review: https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/141877/

Please review now. I got all calls to ProblemReporter.conflictingPackagesFromModules() (as reported by Call Hierrachy).
Do we want a test?

Since we are fixing one specific bug with the change, I guess we can partially revert the change - as long as the debugger works as expected. Or better, as Andrey already mentioned, maybe we should replace the change with changes from #424 . Though I've not looked into the PR/issue in detail.

I guess we could go with suggestion 2. here, if the fix for #424 doesn't help with that. Its not optimal to have debug evaluation snippets that don't work but should.

[stephan-herrmann]

#424 has been merged a while ago.

Has anyone started to work on reconciling both changes?

[trancexpress]

#424 has been merged a while ago.

Has anyone started to work on reconciling both changes?

Sorry, no. I'll look into reverting our changes and setting the option from #424 for the debugger compile. Hopefully I'll find time for this next week.

[trancexpress]

Using the new option seems to make the test happy (tested also with no option at all, the test fails as expected), so I've set: eclipse-jdt/eclipse.jdt.debug#261

After we merge that, we can proceed with reverting changes for https://bugs.eclipse.org/bugs/show_bug.cgi?id=543604 (aside from added test).

[iloveeclipse]

Using the new option seems to make the test happy (tested also with no option at all, the test fails as expected), so I've set: eclipse-jdt/eclipse.jdt.debug#261

Unfortunately this change caused regression in debugger, see eclipse-jdt/eclipse.jdt.debug#309

[trancexpress]

Using the new option seems to make the test happy (tested also with no option at all, the test fails as expected), so I've set: eclipse-jdt/eclipse.jdt.debug#261

Unfortunately this change caused regression in debugger, see eclipse-jdt/eclipse.jdt.debug#309

Did we fix the bug just for user code that is not in a module?

I wonder if we should try to avoid the java package name in the debugger evaluation code. I'm not sure why I gave up on that for the original bug fix... Maybe we can avoid the bad package only for some cases...