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: fix error title regarding private constructors #65250

Merged
merged 2 commits into from
Oct 13, 2019

Conversation

da-x
Copy link
Member

@da-x da-x commented Oct 9, 2019

One reason is that constructors can be private while their types can be
public.

Idea credit to @petrochenkov, discussed at #65153

@rust-highfive
Copy link
Collaborator

r? @varkor

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2019
@da-x
Copy link
Member Author

da-x commented Oct 9, 2019

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned varkor Oct 9, 2019
@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 9, 2019

ping @rust-lang/wg-diagnostics on whether the new wording looks better on average or not, I'm not sure.
(Ignoring "struct variant constructor", which is a clear regression #65250 (comment).)

Perhaps a flag parameter can be added to DefKind::descr to sometimes add "constructor" and sometimes not, and all uses of it will need to be audited, but maybe that's an overkill.

@rust-highfive
Copy link
Collaborator

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-09T20:38:08.1523022Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-09T20:38:08.1616667Z ##[command]git config gc.auto 0
2019-10-09T20:38:08.1697636Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-09T20:38:08.1757983Z ##[command]git config --get-all http.proxy
2019-10-09T20:38:08.1907886Z ##[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/65250/merge:refs/remotes/pull/65250/merge
---
2019-10-09T20:43:49.8575369Z    Compiling serde_json v1.0.40
2019-10-09T20:43:51.5864310Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-10-09T20:44:02.9250192Z     Finished release [optimized] target(s) in 1m 28s
2019-10-09T20:44:02.9340719Z tidy check
2019-10-09T20:44:03.1374302Z tidy error: /checkout/src/test/ui/issues/issue-pr29383.rs:9: line longer than 100 chars
2019-10-09T20:44:03.1375401Z tidy error: /checkout/src/test/ui/issues/issue-pr29383.rs:10: line longer than 100 chars
2019-10-09T20:44:03.1380019Z tidy error: /checkout/src/test/ui/issues/issue-63983.rs:9: line longer than 100 chars
2019-10-09T20:44:03.1380704Z tidy error: /checkout/src/test/ui/issues/issue-63983.rs:11: line longer than 100 chars
2019-10-09T20:44:03.1507318Z tidy error: /checkout/src/test/ui/issues/issue-32004.rs:11: line longer than 100 chars
2019-10-09T20:44:03.1997117Z tidy error: /checkout/src/test/ui/empty/empty-struct-tuple-pat.rs:30: line longer than 100 chars
2019-10-09T20:44:03.1998944Z tidy error: /checkout/src/test/ui/empty/empty-struct-tuple-pat.rs:34: line longer than 100 chars
2019-10-09T20:44:03.2006534Z tidy error: /checkout/src/test/ui/empty/empty-struct-braces-pat-1.rs:25: line longer than 100 chars
2019-10-09T20:44:03.2006859Z tidy error: /checkout/src/test/ui/empty/empty-struct-braces-pat-1.rs:32: line longer than 100 chars
2019-10-09T20:44:03.2007156Z tidy error: /checkout/src/test/ui/empty/empty-struct-unit-pat.rs:21: line longer than 100 chars
2019-10-09T20:44:03.2007422Z tidy error: /checkout/src/test/ui/empty/empty-struct-unit-pat.rs:24: line longer than 100 chars
2019-10-09T20:44:03.2008329Z tidy error: /checkout/src/test/ui/empty/empty-struct-unit-pat.rs:27: line longer than 100 chars
2019-10-09T20:44:03.2008642Z tidy error: /checkout/src/test/ui/empty/empty-struct-unit-pat.rs:30: line longer than 100 chars
2019-10-09T20:44:03.2008940Z tidy error: /checkout/src/test/ui/empty/empty-struct-unit-pat.rs:34: line longer than 100 chars
2019-10-09T20:44:03.2009220Z tidy error: /checkout/src/test/ui/empty/empty-struct-unit-pat.rs:42: line longer than 100 chars
2019-10-09T20:44:03.2193221Z tidy error: /checkout/src/test/ui/pattern/pat-tuple-overfield.rs:11: line longer than 100 chars
2019-10-09T20:44:03.2193491Z tidy error: /checkout/src/test/ui/pattern/pat-tuple-overfield.rs:13: line longer than 100 chars
2019-10-09T20:44:03.2193743Z tidy error: /checkout/src/test/ui/pattern/pattern-error-continue.rs:18: line longer than 100 chars
2019-10-09T20:44:03.2193972Z tidy error: /checkout/src/test/ui/pattern/pattern-binding-disambiguation.rs:52: line longer than 100 chars
2019-10-09T20:44:03.2194196Z tidy error: /checkout/src/test/ui/pattern/pattern-binding-disambiguation.rs:53: line longer than 100 chars
2019-10-09T20:44:03.2234329Z tidy error: /checkout/src/test/ui/privacy/privacy5.rs:104: line longer than 100 chars
2019-10-09T20:44:03.2234587Z tidy error: /checkout/src/test/ui/privacy/privacy5.rs:105: line longer than 100 chars
2019-10-09T20:44:03.2483090Z tidy error: /checkout/src/test/ui/ufcs/ufcs-partially-resolved.rs:28: line longer than 100 chars
2019-10-09T20:44:03.2668383Z tidy error: /checkout/src/test/ui/match/match-pattern-field-mismatch.rs:11: line longer than 100 chars
2019-10-09T20:44:04.8225328Z some tidy checks failed
2019-10-09T20:44:04.8225542Z Found 482 error codes
2019-10-09T20:44:04.8225974Z Found 0 error codes with no tests
2019-10-09T20:44:04.8226018Z Done!
2019-10-09T20:44:04.8226018Z Done!
2019-10-09T20:44:04.8232626Z 
2019-10-09T20:44:04.8233136Z 
2019-10-09T20:44:04.8234143Z 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-09T20:44:04.8234348Z 
2019-10-09T20:44:04.8234371Z 
2019-10-09T20:44:04.8246978Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-10-09T20:44:04.8247374Z Build completed unsuccessfully in 0:01:31
2019-10-09T20:44:04.8247374Z Build completed unsuccessfully in 0:01:31
2019-10-09T20:44:04.8299229Z == clock drift check ==
2019-10-09T20:44:04.8314591Z   local time: Wed Oct  9 20:44:04 UTC 2019
2019-10-09T20:44:04.9280534Z   network time: Wed, 09 Oct 2019 20:44:04 GMT
2019-10-09T20:44:04.9288066Z == end clock drift check ==
2019-10-09T20:44:05.8701926Z ##[error]Bash exited with code '1'.
2019-10-09T20:44:05.8745025Z ##[section]Starting: Checkout
2019-10-09T20:44:05.8746785Z ==============================================================================
2019-10-09T20:44:05.8746830Z Task         : Get sources
2019-10-09T20:44:05.8746888Z 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)

@estebank
Copy link
Contributor

estebank commented Oct 10, 2019

IMO, there are a handful of places where mentioning "constructor" would look good, namely when differentiating between the ADT itself and its constructor in the same message (type mismatch, "expected struct of type foo, found struct constructor foo", but those already look reasonable), but the great majority of these changes look worse/less understandable to me.

The only case I think needs fixing is

error[E0308]: mismatched types
 --> src/main.rs:4:18
  |
1 | struct Foo();
  | ------------- fn() -> Foo {Foo} defined here
...
4 |     let _: Foo = Foo;
  |                  ^^^
  |                  |
  |                  expected struct `Foo`, found fn item
  |                  help: use parentheses to instantiate this tuple struct: `Foo()`
  |
  = note: expected type `Foo`
             found type `fn() -> Foo {Foo}`

which should probably look closer to

error[E0308]: mismatched types
 --> src/main.rs:4:18
  |
1 | struct Foo();
  | ------------- `Foo` defined here
...
4 |     let _: Foo = Foo;
  |                  ^^^
  |                  |
  |                  expected struct `Foo`, found struct `Foo`'s constructor
  |                  help: use parentheses to instantiate this tuple struct: `Foo()`
  |
  = note: expected type `Foo`
             found constructor for type `Foo`

@petrochenkov
Copy link
Contributor

So, I looked through the diff and it's indeed mostly a regression.
It's useful to mention "constructor" only the type and the constructor are opposed to each other.
The "x is private" is the only context where I found that to be the case.

So, I think, let's keep hir/def.rs alone and add "constructor" to the primary message wording only for the privacy errors (same code as in 48f8bed), and only if we are adding the "tuple struct constructor is private" label.

@petrochenkov 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 Oct 11, 2019
The constructor is private, not the type.

Idea credit to @petrochenkov, discussed at rust-lang#65153
@da-x da-x changed the title hir: in error messages, differentiate constructors and types resolve: fix error title regarding private constructors Oct 11, 2019
@da-x
Copy link
Member Author

da-x commented Oct 11, 2019

Fixed.

@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Oct 11, 2019

📌 Commit 9d11bda has been approved by petrochenkov

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 11, 2019
@petrochenkov
Copy link
Contributor

Meta: it's undesirable to @mention people in commit messages, such commits will generate a lot of mail while going through CI.

Centril added a commit to Centril/rust that referenced this pull request Oct 13, 2019
…nkov

resolve: fix error title regarding private constructors

One reason is that constructors can be private while their types can be
public.

Idea credit to @petrochenkov, discussed at rust-lang#65153
bors added a commit that referenced this pull request Oct 13, 2019
Rollup of 13 pull requests

Successful merges:

 - #65039 (Document missing deny by default lints)
 - #65069 (Implement Clone::clone_from for VecDeque)
 - #65165 (Improve docs on some char boolean methods)
 - #65248 (Suggest `if let` on `let` refutable binding)
 - #65250 (resolve: fix error title regarding private constructors)
 - #65295 (Move diagnostics code out of the critical path)
 - #65320 (Report `CONST_ERR` lint in external macros)
 - #65327 (replace the hand-written binary search with the library one)
 - #65339 (do not reference LLVM for our concurrency memory model)
 - #65357 (syntax: simplify maybe_annotate_with_ascription)
 - #65358 (simplify maybe_stage_features)
 - #65359 (simplify integer_lit)
 - #65360 (mbe: reduce panictry! uses.)

Failed merges:

r? @ghost
@bors bors merged commit 9d11bda into rust-lang:master Oct 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants