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

Better context info in Type_Error raised from return type checks #8566

Merged
merged 22 commits into from
Dec 20, 2023

Conversation

radeusgd
Copy link
Member

Pull Request Description

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.

@radeusgd radeusgd self-assigned this Dec 16, 2023
@radeusgd radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Dec 16, 2023
@radeusgd
Copy link
Member Author

Example:

from Standard.Base import all

go x -> Integer =
    if x == 0 then 1 else
        if x == 10 then "TXT" else
            @Tail_Call go x-1

main =
    IO.println <| go 4
    IO.println <| go 12

yields:

1
Execution finished with an error: Type error: expected `go` result to be Integer, but got Text.
        at <enso> test.go(test.enso:3-6)
        at <enso> test.main(test.enso:10:5-23)

@radeusgd
Copy link
Member Author

radeusgd commented Dec 16, 2023

This is a simple approach to ensuring better error messages - when creating the type ascriptions that enforce the type checks, we add a 'comment' to it that will then drive the error message.

I was thinking of implementing a slightly cleaner approach where instead of adding the comment to the IR, we could deduce this in an IR pass from context (detecting the type check at the return of a function), but this seemed like a much simpler solution.

Still I can try to do the 'better' one if we think this is not good.

Alternative solution

I think the toher solution may need then to actually change the overall appproach to type checks - remove the Type.Ascriptions added for return checks, instead adding some return type information to the IR of particular Functions or Methods. Then that type info can be used in a separate ReturnType pass that would generate the ReadArgumentCheckNode with proper comments.


I think regardless of this PR, I will try to experiment a bit with the current approach and see if this approach works well with the next steps of most basic type inference/propagation/checking. If I see that actually there the 'Alternative Solution' would be more desirable anyway, that may mean we should take it and scratch this PR - that's why I separated it from the base PR.

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Computing a "comment" automatically and attaching it to IR is fine. There is #8419 to start an effort of checking what's the fastest thing for runtime-compiler to do.

fail("Expecting an exception, not: " + res);
} catch (PolyglotException e) {
assertContains("expected `expression` to be Integer, but got Text", e.getMessage());
assertContains("expected `x` result to be Integer, but got Text", e.getMessage());
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 be expecting 'x' to return Integer, but got Text?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I think that form may be more readable, I will update it.

Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

This definitely feels like a better error message for an unexpected return type from a method. I have even a better suggestion, I think.

@@ -872,7 +872,7 @@ public void returnTypeCheckOptInError() throws Exception {
var res = plusChecked.execute("a", "b");
fail("Expecting an exception, not: " + res);
} catch (PolyglotException e) {
assertContains("expected `expression` to be Integer, but got Text", e.getMessage());
assertContains("expected `plusChecked` result to be Integer, but got Text", e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

What about the following error message:

expected the result of `plusChecked` method to be Integer, but got Text

Isn't that even better?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure it sounds good to me, I will amend when I will get around to moving this PR forward, thanks for the suggestion

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 just think that maybe function may be a more universal name here?

method fits well for Type.foo methods. But internal helper functions e.g.:

foo x =
    go i = ...
    go 0

are not really _method_s, are they?

So maybe

expected the result of `plusChecked` function to be Integer, but got Text

?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know. I thought that everything function-like in Enso is just called method.

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 did

expected the result of `plusChecked` to be Integer, but got Text

avoiding the method/function controversy.

IMO it is readable.

Base automatically changed from wip/radeusgd/8240-check-return-type to develop December 19, 2023 15:32
@radeusgd radeusgd force-pushed the wip/radeusgd/8240-return-type-better-error branch from 75cf612 to a40cd56 Compare December 19, 2023 15:34
@radeusgd radeusgd added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Dec 19, 2023
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Dec 19, 2023
@mergify mergify bot merged commit dfdb547 into develop Dec 20, 2023
30 checks passed
@mergify mergify bot deleted the wip/radeusgd/8240-return-type-better-error branch December 20, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants