Skip to content
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

Merged
merged 8 commits into from
Nov 29, 2023

Conversation

niloc132
Copy link
Contributor

@niloc132 niloc132 commented Feb 8, 2023

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:

  • When concatenating the result of the ternary to a string
  • When assigning a local field typed with 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

@niloc132 niloc132 added the bug label Feb 8, 2023
@@ -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);
Copy link
Member

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?

Suggested change
CompilationProblemReporter.logAndTranslateException(getTopLogger(), e);
throw CompilationProblemReporter.logAndTranslateException(getTopLogger(), e);

Copy link
Contributor Author

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.

@dnouls
Copy link

dnouls commented Mar 17, 2023

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:

-- Class Scope ---
Member type : LocalizedPrinterParser (id=NoId)
static final class java.time.format.DateTimeFormatterBuilder$LocalizedPrinterParser
            extends java.lang.Object
            implements : java.time.format.DateTimeFormatterBuilder.DateTimePrinterParser
            enclosing type : java.time.format.DateTimeFormatterBuilder
/*   fields   */
private final java.time.format.FormatStyle dateStyle
private final java.time.format.FormatStyle timeStyle
/*   methods   */
void <init>(java.time.format.FormatStyle, java.time.format.FormatStyle)
private java.time.format.DateTimeFormatter formatter(java.util.Locale, java.time.chrono.Chronology)
public int parse(java.time.format.DateTimeParseContext, java.lang.CharSequence, int)
public boolean print(java.time.format.DateTimePrintContext, java.lang.StringBuilder)
public java.lang.String toString()

and this was the toString() method that was being processed:

public @Override String toString() {
  return (((("Localized(" + ((dateStyle != null) ? dateStyle : "")) + ",") + ((timeStyle != null) ? timeStyle : "")) + ")");
}

Copy link

@dnouls dnouls left a 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

complete this todo ?

Copy link
Contributor Author

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'));
Copy link

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"))

Copy link
Contributor Author

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.

@niloc132
Copy link
Contributor Author

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

@niloc132 niloc132 added this to the 2.11 milestone Nov 16, 2023
@niloc132
Copy link
Contributor Author

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.

@niloc132
Copy link
Contributor Author

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.

@niloc132 niloc132 merged commit ffe6183 into gwtproject:main Nov 29, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants