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

NLL diagnostics span RHS of assignment when LHS was prior (and better?) choice #51217

Closed
pnkfelix opened this issue May 30, 2018 · 6 comments
Closed
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. NLL-diagnostics Working towards the "diagnostic parity" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

Consider the output diagnostics from

LL | let x = 42;
| -- first assignment to `x`

and compare them to

LL | let x = 42;
| - first assignment to `x`

I don't know why NLL deviates.

(Maybe its fine to switch to the right-hand side, but I want to make sure we do so as a deliberate choice, not an accident.)

@matthewjasper matthewjasper self-assigned this May 30, 2018
@matthewjasper matthewjasper added A-NLL Area: Non-lexical lifetimes (NLL) NLL-diagnostics Working towards the "diagnostic parity" goal labels May 30, 2018
@matthewjasper
Copy link
Contributor

So let x = 6 * 9; is a single statement in MIR, so there is only currently the option to place one span on it. Most of the time either the current span is the correct choice here, so it's certainly easier to keep the current span. I guess the two main possibilities here are:

  • Check if the variable starts initialized, and use the span from the variable's declaration instead of from the assignment.
  • Always evaluate the RHS into a temporary so that there can be two separate spans: this would break some other stuff I'm working on, and I don't really want MIR to have even more temporaries.

@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 4, 2018

I personally like the option of "Check if the variable starts initialized, and use the span from the variable's declaration instead of from the assignment."

In particular, I'm already extending mir:LocalDecl in a number of ways, threading state down from the AST into each mir::LocalDecl in order to increase the amount of information available for the MIR borrowck to use when constructing diagnostic output.

So it seems natural to me to include information like: Is there an explicit type attached? (if so, what is its span?), and is there an initialization expression (and if so, what is its span)?

@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 4, 2018

(Adding such info as mentioned in my previous comment to mir::LocalDecl may be a natural addition to PR #51275 ; I'm noting that here mostly as a reminder that I should double check that theory as part of my work on that PR.)

@jkordish jkordish added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-nll labels Jun 19, 2018
@nikomatsakis
Copy link
Contributor

I'm marking this as RC -- it seems not that criticial.

@nikomatsakis
Copy link
Contributor

Well, it is somewhat wrong. I'm going to put it as EP.

@nikomatsakis
Copy link
Contributor

But if we can't find a nice fix, oh well.

@nikomatsakis nikomatsakis added this to the Rust 2018 Preview 2 milestone Jun 29, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 2, 2018
…r=nikomatsakis

[NLL] Use better span for initializing a variable twice

Closes rust-lang#51217

When assigning to a (projection from a) local immutable local which starts initialised (everything except `let PATTERN;`):

* Point to the declaration of that local
* Make the error message refer to the local, rather than the projection.

r? @nikomatsakis
bors added a commit that referenced this issue Jul 3, 2018
[NLL] Use better span for initializing a variable twice

Closes #51217

When assigning to a (projection from a) local immutable local which starts initialised (everything except `let PATTERN;`):

* Point to the declaration of that local
* Make the error message refer to the local, rather than the projection.

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. NLL-diagnostics Working towards the "diagnostic parity" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants