Don't suggest using fields in a static method #33615

GuillaumeGomez · 2016-05-13T15:20:09Z

rust-highfive · 2016-05-13T15:20:20Z

(rust_highfive has picked a reviewer for you, use r? to override)

LeoTestard · 2016-05-13T15:48:33Z

src/librustc_resolve/lib.rs

+                                        } else {
+                                            "".to_string()
+                                        }
+                                    }


I find this whole thing is a bit unclear. Basically we are trying to detect things based on what fields there are in the struct rather than in what kind of expression the error occurs. What you're doing, if I understand correctly, is that you emit the help message when trying to refer to a variable named path_name only if we are in a static method of a type that has a field of name path_name, wether we are in the context of a call expression or not (which is fine, the type could have a closure field... ).

But you don't catch the cases where the user would try to invoke a method from the type... Maybe this is the right thing to do since good reporting would require remembering that we are in a call-expression context, which may be a bit overkill. But I don't know... the thing is that if the type has both an field and a method named foo, then foo(...) will spawn the help message saying that you don't have access to the fields, but if you have only a method named foo and no such field, you will not have the message. All of this seems a bit inconsistent. But maybe it's not that much of a problem...

Just updating the help message to include non-static methods as well would solve it I guess.

GuillaumeGomez · 2016-05-15T00:33:54Z

Updated.

@LeoTestard: Does the error message seem better to you or still not good?

bors · 2016-05-16T09:52:10Z

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

birkenfeld · 2016-05-17T05:03:48Z

src/librustc_resolve/lib.rs

-                UnresolvedNameContext::Other => { } // no help available
+                UnresolvedNameContext::Other => {
+                    if msg.is_empty() && is_static_method && is_field {
+                        err.help("This is a static method, you don't have access to \


messages usually start lowercase, no?

Also I remember @steveklabnik saying that "static method" is not supposed to be used, but "associated function".

messages usually start lowercase, no?

Indeed.

Also I remember @steveklabnik saying that "static method" is not supposed to be used, but "associated function".

We'll have to wait for confirmation.

Yes, any fn declared in an impl block is an associated function, and we call an associated function that takes a self a "method".

LeoTestard · 2016-05-19T09:55:51Z

src/librustc_resolve/lib.rs

@@ -1665,7 +1663,7 @@ impl<'a> Resolver<'a> {
                                    let type_parameters =
                                        HasTypeParameters(&sig.generics,
                                                          FnSpace,
-                                                          MethodRibKind);
+                                                          MethodRibKind(false));


I'm not sure if it's useful but in doubt, it's certainly more correct to tell wether the method is static here, rather than just using false. You have a MethodSig so it should be trivial.

GuillaumeGomez · 2016-05-20T21:19:06Z

Updated.

GuillaumeGomez · 2016-05-24T08:49:45Z

r? @pnkfelix

pnkfelix · 2016-05-24T15:13:12Z

src/librustc_resolve/lib.rs

@@ -148,7 +148,7 @@ enum ResolutionError<'a> {
    /// error E0424: `self` is not available in a static method
    SelfNotAvailableInStaticMethod,
    /// error E0425: unresolved name
-    UnresolvedName(&'a str, &'a str, UnresolvedNameContext<'a>),
+    UnresolvedName(&'a str, &'a str, UnresolvedNameContext<'a>, bool, bool),


You should either:

document what each of these bool's represents in a comment above this variant. (I know I can infer it by grepping for the other uses of UnresolvedName, but I don't like having to do that to make sense of this definition), OR

alternative 1: use a struct-style variant (i.e. named fields) instead of a tuple-style variant here, so that you can name each piece of state, or

alternative 2: encode the significance of each bool here in the type.

(there are many ways to do this, such as introducing two new two variant enums, or introducing two structs that each just wrap around bool, so that the name of the struct dictates its significance.)

pnkfelix · 2016-05-24T15:15:26Z

@GuillaumeGomez this code looks fine; r=me after you address my nit regarding the way you are representing the boolean state being added to each variant.

Manishearth · 2016-05-24T16:12:12Z

src/librustc_resolve/lib.rs

-                UnresolvedNameContext::Other => { } // no help available
+                UnresolvedNameContext::Other => {
+                    if msg.is_empty() && is_static_method && is_field {
+                        err.help("this is an associated function, you don't have access to \


If I understand correctly, this is only shown if there is a field by that name?

In that case, we should instead say something like "This is a static (non-self) method, and does not have access to fields. If you wished to access self.{fieldname} you should add a self parameter to the method".

Also, in this case, we should not suggest self.fieldname directly -- I don't think this change makes that happen?

If I understand correctly, this is only shown if there is a field by that name?

Or a method. So what follows your comment cannot really apply.

Also, in this case, we should not suggest self.fieldname directly -- I don't think this change makes that happen?

It does remove it.

GuillaumeGomez · 2016-05-24T17:58:42Z

@bors: r=pnkfelix

bors · 2016-05-24T17:58:43Z

📌 Commit 145747e has been approved by @pnkfelix

bors · 2016-05-24T17:58:54Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Rollup of 7 pull requests #33833

bors · 2016-05-24T17:58:54Z

📌 Commit 145747e has been approved by pnkfelix

bors · 2016-05-25T04:52:03Z

⌛ Testing commit 145747e with merge 487de42...

@LeoTestard

Don't suggest using fields in a static method Fixes #33613. cc @LeoTestard

bors · 2016-05-25T09:20:29Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

rust-highfive assigned Aatch May 13, 2016

LeoTestard reviewed May 13, 2016
View reviewed changes

GuillaumeGomez force-pushed the field_static_method branch 2 times, most recently from 9209cb4 to ec6877a Compare May 15, 2016 10:16

GuillaumeGomez force-pushed the field_static_method branch from ec6877a to 929d6ac Compare May 16, 2016 11:31

birkenfeld reviewed May 17, 2016
View reviewed changes

GuillaumeGomez force-pushed the field_static_method branch 6 times, most recently from 3424c47 to 5f7c7dd Compare May 18, 2016 18:20

LeoTestard reviewed May 19, 2016
View reviewed changes

GuillaumeGomez force-pushed the field_static_method branch from 5f7c7dd to fa338ee Compare May 20, 2016 21:18

Don't suggest using fields in a static method

145747e

rust-highfive assigned pnkfelix and unassigned Aatch May 24, 2016

pnkfelix reviewed May 24, 2016
View reviewed changes

Manishearth reviewed May 24, 2016
View reviewed changes

GuillaumeGomez force-pushed the field_static_method branch 3 times, most recently from fa338ee to 145747e Compare May 24, 2016 17:46

bors added a commit that referenced this pull request May 25, 2016

Auto merge of #33615 - GuillaumeGomez:field_static_method, r=pnkfelix

487de42

Don't suggest using fields in a static method Fixes #33613. cc @LeoTestard

bors merged commit 145747e into rust-lang:master May 25, 2016

GuillaumeGomez deleted the field_static_method branch May 25, 2016 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't suggest using fields in a static method #33615

Don't suggest using fields in a static method #33615

GuillaumeGomez commented May 13, 2016

rust-highfive commented May 13, 2016

LeoTestard May 13, 2016

GuillaumeGomez May 14, 2016

GuillaumeGomez commented May 15, 2016

bors commented May 16, 2016

birkenfeld May 17, 2016

GuillaumeGomez May 17, 2016

steveklabnik May 17, 2016

LeoTestard May 19, 2016

GuillaumeGomez commented May 20, 2016

GuillaumeGomez commented May 24, 2016

pnkfelix May 24, 2016 •

edited

Loading

pnkfelix commented May 24, 2016

Manishearth May 24, 2016

Manishearth May 24, 2016

GuillaumeGomez May 24, 2016

GuillaumeGomez commented May 24, 2016 •

edited

Loading

bors commented May 24, 2016

bors commented May 24, 2016

bors commented May 24, 2016

bors commented May 25, 2016

bors commented May 25, 2016

Don't suggest using fields in a static method #33615

Don't suggest using fields in a static method #33615

Conversation

GuillaumeGomez commented May 13, 2016

rust-highfive commented May 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented May 15, 2016

bors commented May 16, 2016

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

GuillaumeGomez commented May 20, 2016

GuillaumeGomez commented May 24, 2016

pnkfelix May 24, 2016 • edited Loading

Choose a reason for hiding this comment

pnkfelix commented May 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented May 24, 2016 • edited Loading

bors commented May 24, 2016

bors commented May 24, 2016

bors commented May 24, 2016

bors commented May 25, 2016

bors commented May 25, 2016

pnkfelix May 24, 2016 •

edited

Loading

GuillaumeGomez commented May 24, 2016 •

edited

Loading