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.

Sign up for GitHub

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

Jump to bottom

Fix rendering of redefined Conversion error #8245

Merged
merged 19 commits into from
Nov 11, 2023
Merged

Fix rendering of redefined Conversion error #8245

Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
16b0433
checkpoint
radeusgd Nov 7, 2023
0353021
adding non-strict tests
radeusgd Nov 7, 2023
91e2e43
nit in comments
radeusgd Nov 7, 2023
24f14d0
WIP strict and non strict runtime tests
radeusgd Nov 7, 2023
d95ed30
add a compiler text and a hotfix to try to move forward
radeusgd Nov 7, 2023
3cb7296
fixing tests
radeusgd Nov 7, 2023
4a378d0
debugging
radeusgd Nov 8, 2023
9e1c03b
fix a showCode
radeusgd Nov 8, 2023
9829e9b
avoid adding a newline to a binding definition
radeusgd Nov 8, 2023
24c0804
remove now unnecessary workaround
radeusgd Nov 8, 2023
9a81242
update tests
radeusgd Nov 8, 2023
7c47839
test naming
radeusgd Nov 8, 2023
a86e32f
fix column numbering in tests - new one seems more accurate actually
radeusgd Nov 8, 2023
7934bc3
check diagnostics in non-strict mode
radeusgd Nov 8, 2023
b9328bd
add test for multiline conversion error - just to be sure
radeusgd Nov 8, 2023
a4b3fea
CR: split non strict tests into separate file, move helper method
radeusgd Nov 9, 2023
6c2db13
remove diag checks
radeusgd Nov 9, 2023
6f653a6
javafmt
radeusgd Nov 9, 2023
2a37e54
add a test
radeusgd Nov 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 1 addition & 15 deletions ...with-polyglot/src/test/java/org/enso/interpreter/test/NonStrictConversionMethodTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,14 @@
public class NonStrictConversionMethodTests extends TestBase {
private static Context nonStrictCtx;

private static final ByteArrayOutputStream out = new ByteArrayOutputStream();

@BeforeClass
public static void initCtx() {
nonStrictCtx = createNonStrictContext();
}

protected static Context createNonStrictContext() {
var context =
defaultContextBuilder().out(out).option(RuntimeOptions.STRICT_ERRORS, "false").build();
defaultContextBuilder().option(RuntimeOptions.STRICT_ERRORS, "false").build();
final Map<String, Language> langs = context.getEngine().getLanguages();
assertNotNull("Enso found: " + langs, langs.get("enso"));
return context;
Expand All @@ -41,15 +39,6 @@ public static void disposeCtx() {
nonStrictCtx.close();
}

@Before
public void resetOutput() {
out.reset();
}

private String getStdOut() {
return out.toString(StandardCharsets.UTF_8);
}

@Test
public void testAmbiguousConversion() {
String src = """
Expand All @@ -65,7 +54,6 @@ public void testAmbiguousConversion() {
""";
Value res = evalModule(nonStrictCtx, src);
assertEquals(42, res.asInt());
MatcherAssert.assertThat(getStdOut(), Matchers.containsString("Unnamed:7:1: error: Ambiguous conversion: Foo.from Bar is defined multiple times in this module."));
}

@Test
Expand All @@ -86,8 +74,6 @@ public void testAmbiguousConversionUsage() {

Value res = evalModule(nonStrictCtx, src);
assertEquals(142, res.asInt());
// But we should still get the diagnostic!
MatcherAssert.assertThat(getStdOut(), Matchers.containsString("Unnamed:7:1: error: Ambiguous conversion: Foo.from Bar is defined multiple times in this module."));
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to remove this check, because currently apparently the diagnostics are not reported at all in the non-strict mode.

e.g.

enso/engine/runtime-compiler/src/main/scala/org/enso/compiler/Compiler.scala

Lines 899 to 918 in 24c0804

def runErrorHandling(
modules: List[Module]
): Unit = {
if (config.isStrictErrors) {
val diagnostics = modules.flatMap { module =>
val errors = gatherDiagnostics(module)
List((module, errors))
}
if (reportDiagnostics(diagnostics)) {
val count =
diagnostics.map(_._2.collect { case e: Error => e }.length).sum
val warnCount =
diagnostics.map(_._2.collect { case e: Warning => e }.length).sum
context.getErr.println(
s"Aborting due to ${count} errors and ${warnCount} warnings."
)
throw new CompilationAbortedException
}
}
}

cc: @JaroslavTulach @hubertp is this preferred for some reason? Do we not report the diagnostics, because non-strict mode is essentially interactive mode and these error messages are supposed to reach the IDE through 'other means' (i.e. errors baked in the IR)? Or is this just an oversight?

My intuition is that actually logging these kinds of diagnostics may make it easier to debug issues when IDE fails to execute something, so I'd be happy to enable them, but I first need to know if there is some reason for not-enabling them that I'm not aware of.

}

}
Loading