Disallow Self in type param defaults of ADTs #64842

pnkfelix · 2019-09-27T13:27:11Z

Fix #61631

(also includes a drive-by fix to a typo in some related diagnostic output.)

rust-highfive · 2019-09-27T13:27:14Z

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

rust-highfive · 2019-09-27T13:27:16Z

r? @varkor

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

pnkfelix · 2019-09-27T13:27:32Z

cc @alexreg

src/librustc_resolve/diagnostics.rs

src/librustc_resolve/error_codes.rs

src/librustc_resolve/late.rs

src/librustc_resolve/lib.rs

src/librustc_resolve/error_codes.rs

GuillaumeGomez

Thanks a lot for writing the long error explanation when creating a new error code! Just a few minor changes and it should be good to go for me.

src/librustc_resolve/error_codes.rs

pnkfelix · 2019-09-30T08:54:24Z

Thanks a lot for writing the long error explanation when creating a new error code!

/me squints at their code

I cannot tell if you're being sarcastic with your use of the word "long". . .but I'll take your thanks as genuine! No problem, happy to get my feet wet with diagnostics again!

GuillaumeGomez · 2019-09-30T11:10:46Z

No no, it's how we call them: long error explanations. No sarcasm here: I'm really happy about this PR!

GuillaumeGomez · 2019-09-30T11:12:46Z

About the long error explanation: generally we appreciate to have an example on how to fix it, but I'm not sure it can be applied here so I approved the PR. :)

Centril · 2019-09-30T23:16:17Z

cc also @petrochenkov

varkor

Implementation looks good; I just have some wording suggestions.

src/librustc_resolve/late.rs

src/librustc_resolve/diagnostics.rs

alexreg · 2019-10-02T00:35:46Z

Looks good to me, thanks for tackling this!

I agree with @varkor's stylistic/wording "nits" too, for that matter. Modulo that, LGTM?

Update: review feedback Update: placate tidy

…elf` (Without this commit, you still get an error (a very similar one, at that), but it complains about use of forward declaration, which is confusing since people do not necessarily think of `Self` as being declared at all.) Update: incorporate review feedback.

rust-highfive · 2019-10-03T12:03:30Z

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-10-03T11:44:04.6880863Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-03T11:44:04.7087589Z ##[command]git config gc.auto 0
2019-10-03T11:44:04.7159737Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-03T11:44:04.7243669Z ##[command]git config --get-all http.proxy
2019-10-03T11:44:05.6512163Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64842/merge:refs/remotes/pull/64842/merge
---
2019-10-03T11:51:21.3438870Z    Compiling serde_json v1.0.40
2019-10-03T11:51:23.1547165Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-10-03T11:51:34.4627502Z     Finished release [optimized] target(s) in 1m 30s
2019-10-03T11:51:34.4730528Z tidy check
2019-10-03T11:51:34.9011316Z tidy error: /checkout/src/librustc_resolve/late.rs:480: line longer than 100 chars
2019-10-03T11:51:36.5148730Z some tidy checks failed
2019-10-03T11:51:36.5155971Z 
2019-10-03T11:51:36.5155971Z 
2019-10-03T11:51:36.5157291Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-10-03T11:51:36.5157779Z 
2019-10-03T11:51:36.5157920Z 
2019-10-03T11:51:36.5162618Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-10-03T11:51:36.5163262Z Build completed unsuccessfully in 0:01:33
2019-10-03T11:51:36.5163262Z Build completed unsuccessfully in 0:01:33
2019-10-03T11:51:36.5214776Z == clock drift check ==
2019-10-03T11:51:36.5230303Z   local time: Thu Oct  3 11:51:36 UTC 2019
2019-10-03T11:51:36.6087604Z   network time: Thu, 03 Oct 2019 11:51:36 GMT
2019-10-03T11:51:36.6091863Z == end clock drift check ==
2019-10-03T11:51:37.9950459Z ##[error]Bash exited with code '1'.
2019-10-03T11:51:37.9993805Z ##[section]Starting: Checkout
2019-10-03T11:51:37.9997167Z ==============================================================================
2019-10-03T11:51:37.9997252Z Task         : Get sources
2019-10-03T11:51:37.9997294Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Update: incorporate review feedback.

pnkfelix · 2019-10-03T20:38:16Z

@bors r=alexreg

bors · 2019-10-03T20:38:17Z

📌 Commit e443e1b has been approved by alexreg

pnkfelix · 2019-10-03T20:38:42Z

@bors rollup

…pe-param-default, r=alexreg Disallow Self in type param defaults of ADTs Fix rust-lang#61631 (also includes a drive-by fix to a typo in some related diagnostic output.)

@ghost

Rollup of 11 pull requests Successful merges: - #61879 (Stabilize todo macro) - #64675 (Deprecate `#![plugin]` & `#[plugin_registrar]`) - #64690 (proc_macro API: Expose `macro_rules` hygiene) - #64706 (add regression test for #60218) - #64741 (Prevent rustdoc feature doctests) - #64842 (Disallow Self in type param defaults of ADTs) - #65004 (Replace mentions of IRC with Discord) - #65018 (Set RUST_BACKTRACE=0 in tests that include a backtrace in stderr) - #65055 (Add long error explanation for E0556) - #65056 (Make visit projection iterative) - #65057 (typo: fix typo in E0392) Failed merges: r? @ghost

eddyb · 2019-10-04T10:43:11Z

src/librustc_resolve/late.rs

+        // rust-lang/rust#61631: The type `Self` is essentially
+        // another type parameter. For ADTs, we consider it
+        // well-defined only after all of the ADT type parameters have
+        // been provided. Therefore, we do not allow use of `Self`


I disagree with the phrasing (although the conclusion is similar) - see #61631 (comment)

In short, Self expands to/aliases Adt<A, B, C, ...>, referring to all the ADTs' parameters, and we shouldn't allow writing Self where we wouldn't allow Adt<A, B, C, ...>.

rust-highfive assigned varkor Sep 27, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 27, 2019

Centril reviewed Sep 27, 2019

View reviewed changes

GuillaumeGomez reviewed Sep 30, 2019

View reviewed changes

src/librustc_resolve/error_codes.rs Show resolved Hide resolved

GuillaumeGomez requested changes Sep 30, 2019

View reviewed changes

src/librustc_resolve/error_codes.rs Outdated Show resolved Hide resolved

src/librustc_resolve/error_codes.rs Show resolved Hide resolved

GuillaumeGomez approved these changes Sep 30, 2019

View reviewed changes

varkor suggested changes Oct 1, 2019

View reviewed changes

pnkfelix added 2 commits October 3, 2019 13:54

The crux of the bug fix.

3a4921c

Update: review feedback Update: placate tidy

Regression tests.

e443e1b

Update: incorporate review feedback.

pnkfelix force-pushed the fix-issue-61631-self-in-type-param-default branch from 7a0593c to e443e1b Compare October 3, 2019 18:22

bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2019

Centril mentioned this pull request Oct 3, 2019

Rollup of 11 pull requests #65072

Closed

tmandry mentioned this pull request Oct 3, 2019

Rollup of 12 pull requests #65075

Closed

tmandry mentioned this pull request Oct 3, 2019

Rollup of 11 pull requests #65076

Merged

bors merged commit e443e1b into rust-lang:master Oct 4, 2019

eddyb reviewed Oct 4, 2019

View reviewed changes

This was referenced Oct 14, 2019

ICE Type parameter I/#1 (I/1) out of range when substituting #59956

Closed

ICE with Self in default generic type argument #59353

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow Self in type param defaults of ADTs #64842

Disallow Self in type param defaults of ADTs #64842

pnkfelix commented Sep 27, 2019 •

edited

Loading

rust-highfive commented Sep 27, 2019

rust-highfive commented Sep 27, 2019

pnkfelix commented Sep 27, 2019

GuillaumeGomez left a comment

pnkfelix commented Sep 30, 2019

GuillaumeGomez commented Sep 30, 2019

GuillaumeGomez commented Sep 30, 2019

Centril commented Sep 30, 2019

varkor left a comment

alexreg commented Oct 2, 2019

rust-highfive commented Oct 3, 2019

pnkfelix commented Oct 3, 2019

bors commented Oct 3, 2019

pnkfelix commented Oct 3, 2019

eddyb Oct 4, 2019

Disallow Self in type param defaults of ADTs #64842

Disallow Self in type param defaults of ADTs #64842

Conversation

pnkfelix commented Sep 27, 2019 • edited Loading

rust-highfive commented Sep 27, 2019

rust-highfive commented Sep 27, 2019

pnkfelix commented Sep 27, 2019

GuillaumeGomez left a comment

Choose a reason for hiding this comment

pnkfelix commented Sep 30, 2019

GuillaumeGomez commented Sep 30, 2019

GuillaumeGomez commented Sep 30, 2019

Centril commented Sep 30, 2019

varkor left a comment

Choose a reason for hiding this comment

alexreg commented Oct 2, 2019

rust-highfive commented Oct 3, 2019

pnkfelix commented Oct 3, 2019

bors commented Oct 3, 2019

pnkfelix commented Oct 3, 2019

eddyb Oct 4, 2019

Choose a reason for hiding this comment

pnkfelix commented Sep 27, 2019 •

edited

Loading