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

Improved error for undefined but used left side of declarations #3451

Merged
merged 3 commits into from
Feb 1, 2020

Conversation

adri326
Copy link
Member

@adri326 adri326 commented Jan 17, 2020

This addresses #2828

Declaring a variable at the end of a function which expects a return value or having generally a variable declaration where the compiler expects to use the return value caused in most cases an error stating that the variable is undefined but its value is used:

actor Main
  new create(env:Env) =>
    confusing_error_msg()
    
  fun confusing_error_msg(): String =>
    var s = "A string."

This bit of code triggers the following error on 0.33.1:

Error:
main.pony:26:9: the left side is undefined but its value is used
    var s = "A string."
        ^
Error:
main.pony:26:11: left side must be something that can be assigned to
    var s = "A string."
          ^

The presence of the second error was also considered to be erroneous.

This patch discriminates in the is_lvalue function from pass/refer.c cases where the left-hand side of the assignment is not an lvalue and cases where it triggers an error. This way, the compiler can tell between the left side not being something that can be assigned to or it having triggered an error.

Additionally, an Info message is triggered when the left side's old value is needed in a declaration.

This is what the error message now looks like:

Error:
main.pony:6:9: the left side is undefined but its value is used
    var s = "A string."
        ^
    Info:
    main.pony:6:5: the previous value of 's' is used because you are trying to use the return value of this var declaration
        var s = "A string."
        ^

The test is just here to verify that the "must be something that can be
assigned to" error does not show up when the left-hand side of the
assignement triggers an error within `is_lvalue`.
@mfelsche
Copy link
Contributor

That is a cool improvement. The previous error message really didn't look like anything to me.

I don't know if it fits an error message, but i think why the compiler gives this message in the first place, which is kind of pretty unintuitive, is that the assignment to s will return its old value, as in a destructive read. It is just accumulated into a var declaration.
To me the compiler complaining here makes sense to m, if I put it this way:

Using a variable declaration as a "return" expression is not allowed as it would result in an undefined value, given an assignment always yields the previous value of the variable you are assigning to.

Just my 2 cents. I am happy with this going through as is as well.

@adri326
Copy link
Member Author

adri326 commented Jan 17, 2020

I agree with you

src/libponyc/pass/refer.c Outdated Show resolved Hide resolved
@SeanTAllen SeanTAllen added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Jan 28, 2020
@SeanTAllen
Copy link
Member

After @jemc's comment is addressed this is good to be squashed and merged. If it's updated in the next couple of days, it can make it into the next release that is coming very soon.

As suggested by @jemc

Co-Authored-By: Joe Eli McIlvain <joe.eli.mac@gmail.com>
@adri326
Copy link
Member Author

adri326 commented Jan 31, 2020

(poke @SeanTAllen) I just committed it - I had no reason not to :)

@SeanTAllen SeanTAllen merged commit b6aa53f into ponylang:master Feb 1, 2020
@SeanTAllen
Copy link
Member

Thanks @adri326

github-actions bot pushed a commit that referenced this pull request Feb 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants