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

resolve: Do not use "resolve"/"resolution" in error messages #38890

Merged
merged 1 commit into from
Jan 13, 2017

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jan 6, 2017

Use less jargon-y wording instead.
cannot find <struct> <S> in <this scope> and cannot find <struct> <S> in <module a::b> are used for base messages (this also harmonizes nicely with "you can import it into scope" suggestions) and not found in <this scope> and not found in <a::b> are used for short labels in fall-back case.
I tweaked some other diagnostics to avoid using "resolve" (see, e.g., librustc_resolve/macros.rs), but haven't touched messages for imports.

Closes #38750
r? @nrc

@GuillaumeGomez
Copy link
Member

Seems good for me. Thanks for your work!

@eddyb
Copy link
Member

eddyb commented Jan 8, 2017

cc @nikomatsakis @jonathandturner

@sophiajt
Copy link
Contributor

sophiajt commented Jan 9, 2017

I like the idea of changing the wording for better readability.

I'm not 100% yet on "value Foo is not declared in this scope". If we're going to change away from resolved (which sounds 👍 to me), do we want to change to declarations and scopes? It seems like we're trading one bit of terminology for another.

Curious to hear what @nikomatsakis thinks, but how does "value Foo is not visible from here" sound? May need a little tweaking but seems to be a little less jargon-y.

@nrc
Copy link
Member

nrc commented Jan 9, 2017

LGTM, I like the wording.

@jonathandturner I feel like 'declared' and 'scope' are not as bad as 'resolution' - the former are programming jargon, but the latter is compiler jargon. So while it is not perfect, it is much better.

I'm wary about using 'visible' since it seems easy to confuse with visibility (is a variable declared in a scope, vs is the variable accessible from that scope).

r=me, but leaving open for a few days for discussion of wording.

@nikomatsakis
Copy link
Contributor

Strong 👍 on shifting away from "resolved". I have to admit I find it hard to imagine not knowing the word "declaration", but I can appreciate that I am biased and that this might well be the case for some users! That said, I don't like the precise wording of "value Foo is not visible from here" -- the problem is that, to me, that implies that Foo exists but there is a privacy violation or something else (maybe a missing use) that is preventing me from "seeing" it. But really Foo might not even be a type that is declared anywhere, right? Maybe something like "cannot find a type named Foo"?

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jan 10, 2017

@nikomatsakis

But really Foo might not even be a type that is declared anywhere, right? Maybe something like "cannot find a type named Foo"?

Compare messages for

mod m {
    pub struct Foo;
}

fn main() {
    Foo;
}
  1. "not in scope"
error[E0425]: value `Foo` is not declared in this scope
 --> test.rs:6:5
  |
6 |     Foo;
  |     ^^^ not in scope
  |
  = help: possible candidate is found in another module, you can import it into scope:
  = help:   `use m::Foo;
  1. "can't find"
error[E0425]: cannot find a value named `Foo`
 --> test.rs:6:5
  |
6 |     Foo;
  |     ^^^ cannot find `Foo`
  |
  = help: possible candidate is found in another module, you can import it into scope:
  = help:   `use m::Foo;

In the second case we tell that we can't find Foo, but in fact we can find Foo in another module and even suggest to import it into scope. So, mentioning "scope" seems desirable.

@eddyb
Copy link
Member

eddyb commented Jan 10, 2017

What about "cannot find ... in scope"? With the notes being about something with that name being "found elsewhere". This seems better than mentioning declarations which can be confusing wrt imports and other things.

@petrochenkov
Copy link
Contributor Author

What about "cannot find ... in scope"?

Base 1: cannot find <struct> <S> in this scope
Label 1: not in scope
Base 2: cannot find <struct> <S> in <module> <a::b>
Label 2: not in <a::b> (???)

LGTM (except for label 2), but I have nothing against "declared" so I like my variant better.

@nikomatsakis
Copy link
Contributor

@petrochenkov one think that gives me pause about "declared" is that I think of a declaration as being something like struct Foo -- but a use statement doesn't feel like a declaration. Therefore, when I see an error "no Foo declared in scope" it suggests to me that the only way to have Foo be a valid name for me to use is for me to declare an item with that name, which is not the case. I think for this reason I prefer "cannot find".

@nikomatsakis
Copy link
Contributor

I think mentioning scope is ok though. So 👍 to the latest iteration from me.

@petrochenkov
Copy link
Contributor Author

I'm still interested in better suggestions for "Label 2".

@sophiajt
Copy link
Contributor

@petrochenkov what an example for label 2?

I see the pattern, but what does it look like a real example?

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jan 10, 2017

@jonathandturner
Code:

mod m {
    pub mod n {
        pub struct Foo;   
    }
}

fn main() {
    let foo = m::Foo;
}

Diagnostics:

error[E0425]: cannot find value `Foo` in module `m` (BASE 2)
 --> test.rs:8:15
  |
8 |     let foo = m::Foo;
  |               ^^^^^^ (LABEL 2)
  |
  = help: possible candidate is found in another module, you can import it into scope:
  = help:   `use m::n::Foo;`

(Note that path segments don't have individual spans now, so the label has to point to the whole path.)

@sophiajt
Copy link
Contributor

I think it's okay to also use "not in scope" here, too

@eddyb
Copy link
Member

eddyb commented Jan 10, 2017

cannot find value m::Foo in module m

I wonder why this isn't/can't be "cannot find value Foo in module m".
As for the LABEL 2, "not found in m" seems reasonable to me.

EDIT:

(Note that path segments don't have individual spans now, so the label has to point to the whole path.)

We should just fix this, it's been bothering me for other reasons (per-segment lifetime/type parameters).

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jan 11, 2017

I wonder why this isn't/can't be "cannot find value Foo in module m".

Oh, that's just a typo (I modified the error message manually), fixed.

We should just fix this

Ok, I'll use "not in m" then, an add new spans later in a separate PR.

@bors
Copy link
Contributor

bors commented Jan 11, 2017

☔ The latest upstream changes (presumably #38925) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

New messages are looking good. 👍

@petrochenkov
Copy link
Contributor Author

Updated.

@nrc
Copy link
Member

nrc commented Jan 12, 2017

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 12, 2017

📌 Commit 2c6fa8a has been approved by nrc

@petrochenkov
Copy link
Contributor Author

Fixed wrongly updated UI test caught by travis
@bors r=nrc

@bors
Copy link
Contributor

bors commented Jan 12, 2017

📌 Commit 2092682 has been approved by nrc

@bors
Copy link
Contributor

bors commented Jan 13, 2017

⌛ Testing commit 2092682 with merge 2768428...

@bors
Copy link
Contributor

bors commented Jan 13, 2017

💔 Test failed - status-appveyor

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jan 13, 2017

dist failure (appveyor s3 bindings)
@bors retry

@bors
Copy link
Contributor

bors commented Jan 13, 2017

⌛ Testing commit 2092682 with merge 31f0c9d...

@bors
Copy link
Contributor

bors commented Jan 13, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Jan 13, 2017

⌛ Testing commit 2092682 with merge 3e5621b...

@bors
Copy link
Contributor

bors commented Jan 13, 2017

⌛ Testing commit 2092682 with merge 1d5fb06...

bors added a commit that referenced this pull request Jan 13, 2017
resolve: Do not use "resolve"/"resolution" in error messages

Use less jargon-y wording instead.
`cannot find <struct> <S> in <this scope>` and `cannot find <struct> <S> in <module a::b>` are used for base messages (this also harmonizes nicely with "you can import it into scope" suggestions) and `not found in <this scope>` and `not found in <a::b>` are used for short labels in fall-back case.
I tweaked some other diagnostics to avoid using "resolve" (see, e.g., `librustc_resolve/macros.rs`), but haven't touched messages for imports.

Closes #38750
r? @nrc
@bors
Copy link
Contributor

bors commented Jan 13, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 1d5fb06 to master...

@bors bors merged commit 2092682 into rust-lang:master Jan 13, 2017
arielb1 pushed a commit to arielb1/rust that referenced this pull request Mar 9, 2017
Give spans to individual path segments in AST

And use these spans in path resolution diagnostics.

The spans are spans of identifiers in segments, not whole segments. I'm not sure what spans are more useful in general, but identifier spans are a better fit for resolve errors.

HIR still doesn't have spans.

Fixes rust-lang#38927 (comment) rust-lang#38890 (comment)

r? @nrc @eddyb
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 10, 2017
Give spans to individual path segments in AST

And use these spans in path resolution diagnostics.

The spans are spans of identifiers in segments, not whole segments. I'm not sure what spans are more useful in general, but identifier spans are a better fit for resolve errors.

HIR still doesn't have spans.

Fixes rust-lang#38927 (comment) rust-lang#38890 (comment)

r? @nrc @eddyb
alexcrichton pushed a commit to arielb1/rust that referenced this pull request Mar 10, 2017
Give spans to individual path segments in AST

And use these spans in path resolution diagnostics.

The spans are spans of identifiers in segments, not whole segments. I'm not sure what spans are more useful in general, but identifier spans are a better fit for resolve errors.

HIR still doesn't have spans.

Fixes rust-lang#38927 (comment) rust-lang#38890 (comment)

r? @nrc @eddyb
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 10, 2017
Give spans to individual path segments in AST

And use these spans in path resolution diagnostics.

The spans are spans of identifiers in segments, not whole segments. I'm not sure what spans are more useful in general, but identifier spans are a better fit for resolve errors.

HIR still doesn't have spans.

Fixes rust-lang#38927 (comment) rust-lang#38890 (comment)

r? @nrc @eddyb
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 12, 2017
Give spans to individual path segments in AST

And use these spans in path resolution diagnostics.

The spans are spans of identifiers in segments, not whole segments. I'm not sure what spans are more useful in general, but identifier spans are a better fit for resolve errors.

HIR still doesn't have spans.

Fixes rust-lang#38927 (comment) rust-lang#38890 (comment)

r? @nrc @eddyb
@petrochenkov petrochenkov deleted the noresolve branch March 16, 2017 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error message regression: "no resolution found"
8 participants