Fix rendering of redefined Conversion error #8245

radeusgd · 2023-11-07T18:10:35Z

Pull Request Description

Fixes Compiler crash when ambiguous from conversions are encountered #7853

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

ideally we should fix the root cause

engine/runtime-parser/src/main/java/org/enso/compiler/core/TreeToIr.java

radeusgd · 2023-11-08T14:23:38Z

...ne/runtime-parser/src/main/scala/org/enso/compiler/core/ir/expression/errors/Redefined.scala

-      s"Method overloads are not supported: ${targetType.map(_.name + ".").getOrElse("")}from " +
+      s"Ambiguous conversion: ${targetType.map(_.name + ".").getOrElse("")}from " +


I think that this error message may be clearer - conversions work a bit differently than method overloads - they do overload the from method, just with different types of that.

What if I accidentaly do something like this:

method x y = x + y method x = x

Would the error message say "Ambiguous conversion" in that case as well? Isn't that misleading? What about trying to distinguish between those two cases - "Ambiguous conversion" VS "Method overload"?

We do distinguish these cases already.

The error message I have edited was only relevant for conversions, method overloads have a separate error class - I assume the error message for conversions was copy-pasted from overloads and that's why it used to say the same thing.

You can see here a test verifying that the behaviour is exactly as you'd expect it to be.

enso/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/OverloadsResolutionErrorTest.scala

Lines 33 to 42 in b9328bd

the[InterpreterException] thrownBy eval(code) should have message

"Compilation aborted due to errors."

val diagnostics = consumeOut

diagnostics

.filterNot(isDiagnosticLine)

.toSet shouldEqual Set(

"Test:4:1: error: Method overloads are not supported: Nothing.foo is defined multiple times in this module."

)

}

Well, the above test was for member methods - so just to be comprehensive I added a test case like the one you mentioned:
2a37e54

.../runtime-with-instruments/src/test/scala/org/enso/interpreter/test/instrument/ReplTest.scala

radeusgd · 2023-11-08T14:34:28Z

...ne/runtime-parser/src/main/scala/org/enso/compiler/core/ir/expression/errors/Redefined.scala

-      s"(Redefined (Conversion ${targetType.map(_.showCode() + ".").getOrElse("")}from $sourceType))"
+      s"(Redefined (Conversion ${targetType.map(_.showCode() + ".").getOrElse("")}from ${sourceType.showCode()}))"


Small irrelevant fix - before printing the redefined conversion as code was leaking a lot of information by using toString instead of showCode for the sourceType. With that it is more compact and closer to what I think showCode is supposed to be.

engine/runtime-with-polyglot/src/test/java/org/enso/interpreter/test/ConversionMethodTests.java

JaroslavTulach · 2023-11-09T08:50:44Z

engine/runtime/src/test/java/org/enso/interpreter/test/TestBase.java

@@ -32,6 +32,14 @@ protected static Context createDefaultContext() {
    return context;
  }

+  protected static Context createNonStrictContext(OutputStream out) {


Is there only single usage of this method? Consider using createDefaultContext and apply the STRICT_ERRORS option to it.

Can I apply options to a Context after it was constructed?

radeusgd · 2023-11-09T11:48:20Z

...me-with-polyglot/src/test/java/org/enso/interpreter/test/NonStrictConversionMethodTests.java

-    // 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."));


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.

Akirathan

minors. Otherwise LGTM.

Akirathan · 2023-11-10T07:47:47Z

...ne/runtime-parser/src/main/scala/org/enso/compiler/core/ir/expression/errors/Redefined.scala

-      s"Method overloads are not supported: ${targetType.map(_.name + ".").getOrElse("")}from " +
+      s"Ambiguous conversion: ${targetType.map(_.name + ".").getOrElse("")}from " +


What if I accidentaly do something like this:

method x y = x + y method x = x

Would the error message say "Ambiguous conversion" in that case as well? Isn't that misleading? What about trying to distinguish between those two cases - "Ambiguous conversion" VS "Method overload"?

engine/runtime-with-polyglot/src/test/java/org/enso/interpreter/test/ConversionMethodTests.java

radeusgd added 11 commits November 8, 2023 12:03

checkpoint

16b0433

adding non-strict tests

0353021

nit in comments

91e2e43

WIP strict and non strict runtime tests

24f14d0

add a compiler text and a hotfix to try to move forward

d95ed30

ideally we should fix the root cause

fixing tests

3cb7296

debugging

4a378d0

fix a showCode

9e1c03b

avoid adding a newline to a binding definition

9829e9b

remove now unnecessary workaround

24c0804

update tests

9a81242

radeusgd force-pushed the wip/radeusgd/fix-from-bug branch from bfe2cea to 9a81242 Compare November 8, 2023 13:45

radeusgd changed the title ~~Fix ambiguous conversion definition~~ Fix rendering of redefined Conversion in a module Nov 8, 2023

radeusgd changed the title ~~Fix rendering of redefined Conversion in a module~~ Fix rendering of redefined Conversion error Nov 8, 2023

radeusgd added 2 commits November 8, 2023 14:54

test naming

7c47839

fix column numbering in tests - new one seems more accurate actually

a86e32f

radeusgd commented Nov 8, 2023

View reviewed changes

engine/runtime-parser/src/main/java/org/enso/compiler/core/TreeToIr.java Show resolved Hide resolved

radeusgd commented Nov 8, 2023

View reviewed changes

.../runtime-with-instruments/src/test/scala/org/enso/interpreter/test/instrument/ReplTest.scala Show resolved Hide resolved

radeusgd added 2 commits November 8, 2023 15:31

check diagnostics in non-strict mode

7934bc3

add test for multiline conversion error - just to be sure

b9328bd

radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 8, 2023

radeusgd self-assigned this Nov 8, 2023

radeusgd marked this pull request as ready for review November 8, 2023 14:33

radeusgd requested review from 4e6, JaroslavTulach, hubertp and Akirathan as code owners November 8, 2023 14:33

radeusgd commented Nov 8, 2023

View reviewed changes

radeusgd requested a review from jdunkerley November 8, 2023 14:35

JaroslavTulach approved these changes Nov 9, 2023

View reviewed changes

radeusgd added 2 commits November 9, 2023 12:18

CR: split non strict tests into separate file, move helper method

a4b3fea

remove diag checks

6c2db13

radeusgd commented Nov 9, 2023

View reviewed changes

javafmt

6f653a6

Akirathan approved these changes Nov 10, 2023

View reviewed changes

unfurl-links bot mentioned this pull request Nov 11, 2023

Speed Standard.Base initialization in simple Hello World example up! #6100

Closed

add a test

2a37e54

radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Nov 11, 2023

mergify bot merged commit f47ef0c into develop Nov 11, 2023
31 of 33 checks passed

mergify bot deleted the wip/radeusgd/fix-from-bug branch November 11, 2023 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rendering of redefined Conversion error #8245

Fix rendering of redefined Conversion error #8245

radeusgd commented Nov 7, 2023 •

edited

Loading

radeusgd Nov 8, 2023

Akirathan Nov 10, 2023

radeusgd Nov 11, 2023

radeusgd Nov 11, 2023 •

edited

Loading

radeusgd Nov 11, 2023

radeusgd Nov 8, 2023

JaroslavTulach Nov 9, 2023

radeusgd Nov 9, 2023

radeusgd Nov 9, 2023

Akirathan left a comment

Akirathan Nov 10, 2023

		s"Method overloads are not supported: ${targetType.map(_.name + ".").getOrElse("")}from " +
		s"Ambiguous conversion: ${targetType.map(_.name + ".").getOrElse("")}from " +

	the[InterpreterException] thrownBy eval(code) should have message
	"Compilation aborted due to errors."

	val diagnostics = consumeOut
	diagnostics
	.filterNot(isDiagnosticLine)
	.toSet shouldEqual Set(
	"Test:4:1: error: Method overloads are not supported: Nothing.foo is defined multiple times in this module."
	)
	}

		s"(Redefined (Conversion ${targetType.map(_.showCode() + ".").getOrElse("")}from $sourceType))"
		s"(Redefined (Conversion ${targetType.map(_.showCode() + ".").getOrElse("")}from ${sourceType.showCode()}))"

		// 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."));

	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
	}
	}
	}

Fix rendering of redefined Conversion error #8245

Fix rendering of redefined Conversion error #8245

Conversation

radeusgd commented Nov 7, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd Nov 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akirathan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd commented Nov 7, 2023 •

edited

Loading

radeusgd Nov 11, 2023 •

edited

Loading