-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle ternary intersection types #9799
Conversation
Also improved output for a specific assertion to see what type is causing an assertion error.
@@ -1092,7 +1091,8 @@ void compileForWebMode(ModuleDef module, Set<String> userAgents) | |||
try { | |||
success = Compiler.compile(getTopLogger(), options, module); | |||
} catch (Exception e) { | |||
getTopLogger().log(Type.ERROR, "Compiler aborted with an exception ", e); | |||
// noinspection ThrowableNotThrown | |||
CompilationProblemReporter.logAndTranslateException(getTopLogger(), e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this throw the returned exception?
CompilationProblemReporter.logAndTranslateException(getTopLogger(), e); | |
throw CompilationProblemReporter.logAndTranslateException(getTopLogger(), e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It very reasonably could, but see one line (logical) line down, an exception will be thrown regardless of whether the compiler returned false, or an exception was thrown and logged.
My goal was to only change the logging rather than the rest of the behavior here.
I hope this PR gets merged for the next GWT release as it fixed a problem that was blocking me from migrating to GWT 2.10.0 (current workaround is to disable assertions, but we like to keep them enabled) My project was failing to build with assertions when depending on gwt-time 2.0.10 (https://github.com/foal/gwt-time/releases/tag/v2.0.10) I retried to build using this branch and everything was compiling and unit tests were all running fine. Some info on what was triggered the bug: I put a breakpoint in IntelliJ on the assert statement and this was the class scope:
and this was the toString() method that was being processed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just helping out on getting this patch merged. I was suffering from this bug in one of my projects. This PR fixes it, so I hope this will land soon in an official GWT release.
@@ -65,6 +67,30 @@ public void testLocalVarType_Anonymous() { | |||
assertEquals("42", o.s); | |||
} | |||
|
|||
//TODO add a test here, in case type inference around var and intersection casts does something irritating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
complete this todo ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, the code that follows is the test that I planned to add, I just forgot to remove the TODO.
@@ -583,6 +583,8 @@ public void testConditionals() { | |||
assertFalse(FALSE ? TRUE : false); | |||
assertFalse(TRUE ? FALSE : true); | |||
assertTrue(FALSE ? FALSE : true); | |||
|
|||
assertEquals("abc", "ab" + (TRUE ? "c" : 'c')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to also test (don't know if it makes a difference internally or not)
assertEquals("abd", "ab"+ (FALSE ? "c": "d"))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll add this check, though really we're just making sure that the compiler correctly looks at these types and finds the correct intersection.
I've run both of these tests by hand, and deliberately changed the strings to force them to fail so that we know that they actually run and a failure would be detected. Unfortunately, as wired, the Java 8/7/10/11 language tests are not running at all, even with Java 11 support from #9620 merged, and even with them hacked in to be forced to run, there is a compiler error in the Java 8 test. This uncompilable source file was last edited (with the compiler error) by me. I'll follow up these PRs with some work to get those not only building but running when appropriate. #9813 |
This requires #9868 to merge (or else some emulation tests will continue to not be run, not fail when broken). I've re-tested all of this with 9868 locally, it should be ready to merge. |
Nightly build with -sourceLevel auto enabled, so we can confirm that the emulation tests pass https://github.com/niloc132/gwt/actions/runs/7032403305. If there are no further comments, I intend to land this patch after that passes. |
Starting in GWT 2.9.0 (check jdt vers), JDT changed how it emitted ternary expressions to result in intersection casts in at least two cases:
var
.This error only previously happened when assertions were enabled - the existing code seems to behave more or less as expected.
This fix allows for those two cases to use intersection types, but since the GWT AST doesn't have support for intersection types, the fix selects the first non-java.lang.Object type in the list of types. This is consistent with what we used to do here, but by changing these specific cases rather than altering JdtUtil.signature() we only permit intersection types in these specific cases.
It is technically possible for a JDT WildcardBinding to show up as an intersection cast, but because JdtUtil.signature() already calls erasure() on the TypeBinding, we will never fail the assertion in this way.
JUnitShell now will log context of where a compilation error occurred, and the specific assertion that previously failed will also log the encountered type, to aid future debugging in this area.
Fixes #9694