Uh oh!
There was an error while loading. Please reload this page.
Suggest parentheses when a struct literal needs them - #51360
Conversation
When writing a struct literal in an expression that expects a block to
be started afterwards (like an `if` statement), do not suggest using the
same struct literal:
```
did you mean `S { /* fields * /}`?
```
Instead, suggest surrounding the expression with parentheses:
```
did you mean `(S { /* fields * /})`?
```rust-highfive
commented
Jun 5, 2018
(rust_highfive has picked a reviewer for you, use r? to override) |
michaelwoerister
commented
Jun 5, 2018
r? @nikomatsakis for re-assignment |
| } else { | ||
| err.span_label(span, format!("did you mean `{} {{ /* fields */ }}`?", | ||
| path_str)); | ||
| // HACK(estebank): find a better way to figure out that this was a |
There was a problem hiding this comment.
This is .. certainly a hack. I don't know what would be the best way to handle it, though; we have an id, so if we had the AST on hand we could find the context for this path by -- at worst -- walking through it. I'm not sure if there is a "map" for the AST that lets you find out (e.g.) the parent node?
That said, this function is invoked (iirc) via a visit walk, so perhaps we could add some kind of "context" parameter and thread the information down.
There was a problem hiding this comment.
The problem is that the AST is already incorrect. I would like to modify the parser to accept struct literals in these positions to minimize errors being shown and include the suggestion then, but wanted to have a quick solution until then as this is a problem I've seen crop up multiple times on the issue tracker and in the forums.
There was a problem hiding this comment.
Well, my point regarding hack was not the approach of making a suggestion, which makes sense to me, but rather using the spans etc.
There was a problem hiding this comment.
OK, re-reading this diff, I see I was a bit confused about what was going on. This now seems more reasonable than I thought at first. =)
nikomatsakis
left a comment
There was a problem hiding this comment.
Maybe break up the test? r=me either way
| | ^^^ did you mean `Foo { /* fields */ }`? | ||
| | ^^^ | ||
| | | | ||
| | did you mean `foo`? |
There was a problem hiding this comment.
I find this new test confusing -- it seems to combine a bunch of stuff that's hard to sort out. It took me a while to see that, by adding foo, you triggered a new suggestion here, for example. Perhaps it'd be better to make a new test file (or even multiple new files)? (Or did you intentionally want to demonstrate this interaction / cover all these cases simultaneously?)
There was a problem hiding this comment.
When I originally introduced that suggestion, it was by accident due to reusing the similar identifier. Once I saw it, I thought it was a good idea to keep it as I'm not sure we're exercising the multiple suggestion scenario anywhere else. If you feel that this warrants breaking up into smaller tests, I can do that.
estebank
commented
Jun 8, 2018
@bors r=nikomatsakis rollup |
bors
commented
Jun 8, 2018
📌 Commit 377cf44 has been approved by |
…cts, r=nikomatsakis
Suggest parentheses when a struct literal needs them
When writing a struct literal in an expression that expects a block to
be started afterwards (like an `if` statement), do not suggest using the
same struct literal:
```
did you mean `S { /* fields * /}`?
```
Instead, suggest surrounding the expression with parentheses:
```
did you mean `(S { /* fields * /})`?
```
Fixrust-lang#47360, rust-lang#50090. Leaving rust-lang#42982 open to come back to this problem with a better solution.Rollup of 13 pull requests Successful merges: - #50143 (Add deprecation lint for duplicated `macro_export`s) - #51099 (Fix Issue 38777) - #51276 (Dedup auto traits in trait objects.) - #51298 (Stabilize unit tests with non-`()` return type) - #51360 (Suggest parentheses when a struct literal needs them) - #51391 (Use spans pointing at the inside of a rustdoc attribute) - #51394 (Use scope tree depths to speed up `nearest_common_ancestor`.) - #51396 (Make the size of Option<NonZero*> a documented guarantee.) - #51401 (Warn on `repr` without hints) - #51412 (Avoid useless Vec clones in pending_obligations().) - #51427 (compiletest: autoremove duplicate .nll.* files (#51204)) - #51436 (Do not require stage 2 compiler for rustdoc) - #51437 (rustbuild: generate full list of dependencies for metadata) Failed merges:
When writing a struct literal in an expression that expects a block to
be started afterwards (like an
ifstatement), do not suggest using thesame struct literal:
Instead, suggest surrounding the expression with parentheses:
Fix#47360, #50090. Leaving #42982 open to come back to this problem with a better solution.