Tweaks to invalid ctor messages #47116

estebank · 2018-01-02T00:40:15Z

Do not suggest using a constructor that isn't accessible
Suggest the appropriate syntax (()/{} as appropriate)
Add note when trying to use Self as a ctor

rust-highfive · 2018-01-02T00:40:25Z

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

estebank · 2018-01-02T18:08:07Z

src/test/ui/resolve/tuple-struct-alias.stderr

-   |                 ^^^^ did you mean `Self { /* fields */ }`?
+   |                 ^^^^ not a function
+   |
+   = note: can't instantiate `Self`, you must use the implemented struct directly


This message would be better if it mentioned the struct S. It'd be even better if it checked that it was a suggestion:

= help: can't instantiate `Self`, you must use the struct `S` directly: | 16 | let s = S(0, 1); //~ ERROR expected function | ^

petrochenkov · 2018-01-07T22:53:19Z

@estebank
Sorry for the delay, I'll review all the PRs in a couple of days.

petrochenkov · 2018-01-13T17:53:05Z

src/librustc_resolve/lib.rs

+                                                     block));
+                        return (err, candidates);
+                    }
+                    (Def::SelfTy(_, _), _) if ns == ValueNS && is_struct_like(def) => {


is_struct_like is always true here.

Nit: _, _ -> ..

petrochenkov · 2018-01-13T17:55:00Z

src/librustc_resolve/lib.rs

+                        return (err, candidates);
+                    }
+                    (Def::SelfTy(_, _), _) if ns == ValueNS && is_struct_like(def) => {
+                        err.note("can't instantiate `Self`, you must use the implemented struct \


I don't think "instantiate" is a correct word here.
Maybe "can't use Self as a constructor"?

Also the suggestion to use Self { ... } with braces is now missing (but maybe it's okay).

Also suggestions to use Foo { ... } with braces are now missing for type aliases.

petrochenkov · 2018-01-13T17:55:23Z

src/librustc_resolve/lib.rs

-                                    err.span_label(span, format!("constructor is not visible \
-                                                                   here due to private fields"));
-                                }
+                    (Def::Struct(def_id), _) if ns == ValueNS && is_struct_like(def) => {


is_struct_like is always true here.

petrochenkov · 2018-01-13T17:59:03Z

src/librustc_resolve/lib.rs

                        return (err, candidates);
                    }
+                    (Def::VariantCtor(_, ctor_kind), _) if ns == ValueNS && is_struct_like(def) => {


This is equivalent to (Def::VariantCtor(_, CtorKind::Fictive), _) if ns == ValueNS

petrochenkov · 2018-01-13T18:10:11Z

src/librustc_resolve/lib.rs

+                                    Def::StructCtor(_, CtorKind::Fn) |
+                                    Def::VariantCtor(_, CtorKind::Fn) => "(/* fields */)",
+                                    Def::StructCtor(_, CtorKind::Fictive) |
+                                    Def::VariantCtor(_, CtorKind::Fictive) => {


Variants ctors are unreachable here and fictive struct ctor is impossible.

Could you also add a test for this suggestion?

So I'm is it correct for me to understand that in this case I should be checking for ctor_def being Def::Struct(..) instead of Def::StructCtor(_, Fictive)?

- Properly address Variant Ctors - Show signature if span of trait method without `self` is not available

petrochenkov · 2018-01-15T21:28:22Z

src/librustc_resolve/lib.rs

+                            if is_expected(ctor_def) && !accessible_ctor {
+                                err.span_label(span, format!("constructor is not visible \
+                                                              here due to private fields"));
+                            } else if accessible_ctor {


Actually, now I don't think this condition is reachable at all.
With ns == Value we would find the constructor before Def::Struct if it were really accessible.

petrochenkov · 2018-01-15T21:31:27Z

src/librustc_resolve/lib.rs

+                                                      path_str));
+                        return (err, candidates);
+                    }
+                    (Def::VariantCtor(_, CtorKind::Fictive), _) => {


These three VariantCtor arms look like they give tautological recommendations, I don't think they are ever useful (at least in realistic code).

petrochenkov · 2018-01-15T21:31:48Z

src/librustc_resolve/lib.rs

                        return (err, candidates);
                    }
+                    (Def::SelfTy(..), _) if ns == ValueNS => {
+                        err.note("can't use `Self` as a constructor, you must use the \
+                                  implemented struct");


Missing return (err, candidates);

petrochenkov · 2018-01-15T21:32:44Z

src/librustc_resolve/lib.rs

+                                  implemented struct");
+                    }
+                    (Def::TyAlias(_def_id), _) => {
+                        err.note("can't use a type alias as a constructor");


Missing return (err, candidates);, missing if ns == ValueNS, unused _def_id.

petrochenkov · 2018-01-16T21:07:33Z

@bors r+

bors · 2018-01-16T21:07:34Z

📌 Commit 282f75a has been approved by petrochenkov

estebank · 2018-01-16T21:45:59Z

@bors r-, there're some diagnostic output I need to fix by re-adding VariantCtor match arms. Is it ok if I "r" it after doing that or should I change the output to match the current code?

petrochenkov · 2018-01-16T23:37:16Z

src/librustc_resolve/lib.rs

+                                  implemented struct");
+                        return (err, candidates);
+                    }
+                    (Def::TyAlias(_), _) if ns == ValueNS => {


Def::AssociatedTy(..) can be added here.

petrochenkov · 2018-01-16T23:38:36Z

src/librustc_resolve/lib.rs

-                                    err.span_label(span, format!("constructor is not visible \
-                                                                   here due to private fields"));
-                                }
+                    (Def::Struct(def_id), _) if ns == ValueNS => {


Def::Union(..), Def::Variant(..) and Def::VariantCtor(_, CtorKind::Fictive) can be added here to restore what the previous code did (they are equivalent to structs without accessible constructors in this context).

Only CtorKind::Fictive is relevant here because for others #47116 (comment) still holds.

petrochenkov · 2018-01-16T23:40:09Z

Some other cases beside VariantCtor are missing too.

kennytm · 2018-01-17T15:33:49Z

@bors r-

The command in #47116 (comment) is ignored since homu treats r-, as a single word 😒

petrochenkov · 2018-01-20T23:18:08Z

@bors r+

bors · 2018-01-20T23:18:09Z

📌 Commit 0674050 has been approved by petrochenkov

bors · 2018-01-21T16:52:16Z

⌛ Testing commit 0674050 with merge 97520cc...

Tweaks to invalid ctor messages - Do not suggest using a constructor that isn't accessible - Suggest the appropriate syntax (`()`/`{}` as appropriate) - Add note when trying to use `Self` as a ctor CC #22488, fix #47085.

bors · 2018-01-21T19:41:19Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 97520cc to master...

rust-highfive assigned michaelwoerister Jan 2, 2018

petrochenkov assigned petrochenkov and unassigned michaelwoerister Jan 2, 2018

estebank force-pushed the non-accessible-ctor branch from 9040c5d to 957d6ef Compare January 2, 2018 06:27

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 2, 2018

estebank force-pushed the non-accessible-ctor branch from 957d6ef to 35ea817 Compare January 2, 2018 17:20

estebank commented Jan 2, 2018

View reviewed changes

petrochenkov reviewed Jan 13, 2018

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 13, 2018

estebank added 5 commits January 15, 2018 02:03

Hide suggestion to use struct ctor when it is not visible

9bab0f0

Suggest the correct syntax for different struct types

a317da4

Readd suggestion in enum variants with incorrect args

ce9ef37

Add note when trying to use Self as a ctor

38546ba

address review comments

445f339

estebank force-pushed the non-accessible-ctor branch 2 times, most recently from 07181b0 to 27d2559 Compare January 15, 2018 19:19

estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 15, 2018

Further tweaks to the output

4a76916

- Properly address Variant Ctors - Show signature if span of trait method without `self` is not available

estebank force-pushed the non-accessible-ctor branch from 27d2559 to 4a76916 Compare January 15, 2018 20:35

petrochenkov reviewed Jan 15, 2018

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 15, 2018

Review comments: remove enum suggestion

282f75a

estebank force-pushed the non-accessible-ctor branch from 40cf9dc to 282f75a Compare January 15, 2018 23:21

petrochenkov added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 16, 2018

petrochenkov reviewed Jan 16, 2018

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 16, 2018

Fix tests by keepeing needed suggestions

0674050

petrochenkov added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 20, 2018

GuillaumeGomez mentioned this pull request Jan 21, 2018

Rollup of 7 pull requests #47636

Closed

bors merged commit 0674050 into rust-lang:master Jan 21, 2018

estebank deleted the non-accessible-ctor branch November 9, 2023 05:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweaks to invalid ctor messages #47116

Tweaks to invalid ctor messages #47116

estebank commented Jan 2, 2018 •

edited

Loading

rust-highfive commented Jan 2, 2018

estebank Jan 2, 2018

petrochenkov commented Jan 7, 2018

petrochenkov Jan 13, 2018

petrochenkov Jan 13, 2018

petrochenkov Jan 13, 2018

petrochenkov Jan 13, 2018

petrochenkov Jan 13, 2018

petrochenkov Jan 13, 2018

petrochenkov Jan 13, 2018

petrochenkov Jan 13, 2018

petrochenkov Jan 13, 2018

estebank Jan 15, 2018 •

edited

Loading

petrochenkov Jan 15, 2018

petrochenkov Jan 15, 2018

petrochenkov Jan 15, 2018

estebank Jan 15, 2018

petrochenkov Jan 15, 2018

estebank Jan 15, 2018

petrochenkov commented Jan 16, 2018

bors commented Jan 16, 2018

estebank commented Jan 16, 2018

petrochenkov Jan 16, 2018

petrochenkov Jan 16, 2018

petrochenkov Jan 16, 2018

petrochenkov commented Jan 16, 2018

kennytm commented Jan 17, 2018

petrochenkov commented Jan 20, 2018

bors commented Jan 20, 2018

bors commented Jan 21, 2018

bors commented Jan 21, 2018

Tweaks to invalid ctor messages #47116

Tweaks to invalid ctor messages #47116

Conversation

estebank commented Jan 2, 2018 • edited Loading

rust-highfive commented Jan 2, 2018

Choose a reason for hiding this comment

petrochenkov commented Jan 7, 2018

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

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

Choose a reason for hiding this comment

estebank Jan 15, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Jan 16, 2018

bors commented Jan 16, 2018

estebank commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Jan 16, 2018

kennytm commented Jan 17, 2018

petrochenkov commented Jan 20, 2018

bors commented Jan 20, 2018

bors commented Jan 21, 2018

bors commented Jan 21, 2018

estebank commented Jan 2, 2018 •

edited

Loading

estebank Jan 15, 2018 •

edited

Loading