Parse alternative incorrect uses of await and recover #60873

estebank · 2019-05-16T02:48:09Z

estebank · 2019-05-16T02:49:25Z

I'll split the code in smaller functions soon, but wanted to get this PR out the door.

I decided against also supporting .await! and .await!() because those two won't be used by anyone that isn't also aware about what the correct syntax is.

src/test/ui/issues/issue-51719.stderr

Centril

:)

Centril · 2019-05-16T09:55:59Z

src/test/ui/issues/issue-51751.stderr

 LL |     let finished = result.await;
-   |                    ^^^^^^^^^^^^
+   |                    ^^^^^^^^^^^^ only allowed inside `async` functions and blocks


I wonder if it would be more understandable to highlight like so:

LL | let finished = result.await; | ^^^^^^ only allowed inside `async` functions and blocks

(if this is something that can be done feasibly within the current diagnostics infra without too many hacks, if not, let's not do that)

Will leave for a later time.

src/test/ui/await-keyword/2018-edition-error-in-non-macro-position.rs

src/test/ui/await-keyword/2018-edition-error.rs

src/libsyntax/parse/parser.rs

Centril · 2019-05-16T10:40:06Z

src/libsyntax/parse/parser.rs

+                    err
+                })?;
+                let expr_str = self.sess.source_map().span_to_snippet(expr.span)
+                    .unwrap_or_else(|_| pprust::expr_to_string(&expr));


This bit is duplicated from above. I have a feeling this pattern occurs frequently so it would be good to consider a cleanup after this PR (but deduplicate here for now).

src/libsyntax/parse/parser.rs

src/test/ui/await-keyword/incorrect-syntax-suggestions.stderr

src/librustc/hir/lowering.rs

cramertj · 2019-05-16T16:49:05Z

src/librustc/hir/lowering.rs

                self.sess,
                await_span,
                E0728,
                "`await` is only allowed inside `async` functions and blocks"
            );
-            self.sess.abort_if_errors();
+            err.span_label(await_span, "only allowed inside `async` functions and blocks");


this seems redundant with the message above

I'm trying to remove all primary spans without labels from the cli output. The main msg should be generic and the label text more targeted and suitable to show inline in (for example) VSCode.

Would you be ok if the main message was "await in a function or block not marked with async" or similar?

I don't think we should have two messages conveying the same information-- the wording isn't the problem.

Some people ignore the error message entirely and focus exclusively on the spans. I agree that unnecessary text is suboptimal, but I do not

error[E0728]: `await` is only allowed inside `async` functions and blocks --> $DIR/issue-51751.rs:11:20 | LL | let finished = result.await; | ^^^^^^^^^^^^

vs

error[E0728]: `await` is only allowed inside `async` functions and blocks --> $DIR/issue-51751.rs:11:20 | LL | let finished = result.await; | ^^^^^^^^^^^^ only allowed inside `async` functions and blocks

vs

error[E0728]: `await` is only allowed inside `async` functions and blocks --> $DIR/issue-51751.rs:11:20 | LL | fn main() { | ---- this is not `async` LL | let result = inc(10000); LL | let finished = result.await; | ^^^^^^^^^^^^ only allowed inside `async` functions and blocks

vs

error[E0728]: `await` is only allowed inside `async` functions and blocks --> $DIR/issue-51751.rs:11:20 | LL | fn main() { | ---- this is not `async` LL | let result = inc(10000); LL | let finished = result.await; | ^^^^^^^^^^^^

The last case is what this would be like if we remove the label, which has some text in the secondary label, but no text on the primary label, which I feel is suboptimal.

@estebank Hmm, ok -- leave it as is then; in context I think your solution is the best UX.

- Change wording of suggestion - Move recovery logic to `diagnostics.rs` - Reduce ammount of code duplication

Centril · 2019-05-16T21:25:19Z

Code looks great now; thanks for doing this!

r=me with green travis :)

estebank · 2019-05-16T21:33:26Z

@Centril take one last look at the last commit that moves some recovery methods out of Parser to diagnostics as a drive-by clean-up.

Centril

Drive by review to your drive by cleanups... ;)

src/libsyntax/parse/diagnostics.rs

Centril

Thanks! r=me with green travis. :)

estebank · 2019-05-16T23:39:31Z

@bors r=Centril

bors · 2019-05-16T23:39:32Z

📌 Commit c084d0e has been approved by Centril

@Centril

Parse alternative incorrect uses of await and recover Fix rust-lang#60613. r? @Centril

@ghost

Rollup of 6 pull requests Successful merges: - #60685 (Switch to SPDX 2.1 license expression) - #60687 (Fix .natvis visualizers.) - #60805 (remove compiletest's dependency on `filetime`) - #60862 (Get ty from local_decls instead of using Place) - #60873 (Parse alternative incorrect uses of await and recover) - #60894 (Add entry-like methods to HashSet) Failed merges: r? @ghost

bors · 2019-05-17T03:52:21Z

⌛ Testing commit c084d0e with merge 1bbb135...

rust-highfive assigned Centril May 16, 2019

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

estebank force-pushed the bad-await branch from fb7963f to 53cd99d Compare May 16, 2019 02:50

estebank changed the title ~~[WIP] Parse alternative incorrect uses of await and recover~~ Parse alternative incorrect uses of await and recover May 16, 2019

This comment has been minimized.

Sign in to view

Xanewok reviewed May 16, 2019

View reviewed changes

src/test/ui/issues/issue-51719.stderr Outdated Show resolved Hide resolved

Centril suggested changes May 16, 2019

View reviewed changes

Mark-Simulacrum 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 May 16, 2019

This comment has been minimized.

Sign in to view

cramertj reviewed May 16, 2019

View reviewed changes

estebank added 7 commits May 16, 2019 13:56

Parse alternative incorrect uses of await and recover

d763faf

Split parser logic to its own method

ee02661

Simplify span usage for incorrect await

01c6689

tidy fix

91c36c4

Point at enclosing fn/closure when it's not async

c616605

readd match await test case

0183a57

Review comments

b9d6fe3

- Change wording of suggestion - Move recovery logic to `diagnostics.rs` - Reduce ammount of code duplication

Centril approved these changes May 16, 2019

View reviewed changes

estebank added 2 commits May 16, 2019 14:30

Fix span for await macro call

27a2881

Move some parser recovery methods to diagnostics

4117c6d

estebank force-pushed the bad-await branch from bea1da5 to 4117c6d Compare May 16, 2019 21:31

Centril suggested changes May 16, 2019

View reviewed changes

src/libsyntax/parse/diagnostics.rs Outdated Show resolved Hide resolved

src/libsyntax/parse/diagnostics.rs Outdated Show resolved Hide resolved

src/libsyntax/parse/diagnostics.rs Show resolved Hide resolved

src/libsyntax/parse/diagnostics.rs Outdated Show resolved Hide resolved

review comments

c084d0e

Centril approved these changes May 16, 2019

View reviewed changes

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 May 16, 2019

Centril added a commit to Centril/rust that referenced this pull request May 17, 2019

Rollup merge of rust-lang#60873 - estebank:bad-await, r=Centril

70b38d1

Parse alternative incorrect uses of await and recover Fix rust-lang#60613. r? @Centril

Centril mentioned this pull request May 17, 2019

Rollup of 6 pull requests #60898

Merged

bors merged commit c084d0e into rust-lang:master May 17, 2019

estebank mentioned this pull request May 29, 2019

Make parser aware of await where macro calls are expected #60829

Closed

estebank deleted the bad-await branch November 9, 2023 05:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse alternative incorrect uses of await and recover #60873

Parse alternative incorrect uses of await and recover #60873

estebank commented May 16, 2019

estebank commented May 16, 2019

This comment has been minimized.

Centril left a comment

Centril May 16, 2019

estebank May 16, 2019

Centril May 16, 2019

This comment has been minimized.

cramertj May 16, 2019

estebank May 16, 2019

cramertj May 16, 2019

estebank May 16, 2019

Centril May 16, 2019

Centril commented May 16, 2019

estebank commented May 16, 2019

Centril left a comment

Centril left a comment

estebank commented May 16, 2019

bors commented May 16, 2019

bors commented May 17, 2019

Parse alternative incorrect uses of await and recover #60873

Parse alternative incorrect uses of await and recover #60873

Conversation

estebank commented May 16, 2019

estebank commented May 16, 2019

This comment has been minimized.

Centril left a 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

This comment has been minimized.

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

Centril commented May 16, 2019

estebank commented May 16, 2019

Centril left a comment

Choose a reason for hiding this comment

Centril left a comment

Choose a reason for hiding this comment

estebank commented May 16, 2019

bors commented May 16, 2019

bors commented May 17, 2019