Emit error for pattern arguments in trait methods #53051

varkor · 2018-08-04T01:22:28Z

The error and check for this already existed, but the parser didn't try to parse trait method arguments as patterns, so the error was never emitted. This surfaces the error, so we get better errors than simple parse errors.

This improves the error message described in #53046.

r? @petrochenkov

rust-highfive · 2018-08-04T01:37:50Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (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.


[00:03:54] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:54] tidy error: /checkout/src/libsyntax/parse/parser.rs:1776: line longer than 100 chars
[00:03:55] some tidy checks failed
[00:03:55] 
[00:03:55] 
[00:03:55] 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" "--quiet"
[00:03:55] 
[00:03:55] 
[00:03:55] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:55] Build completed unsuccessfully in 0:00:55
[00:03:55] Build completed unsuccessfully in 0:00:55
[00:03:55] Makefile:79: recipe for target 'tidy' failed
[00:03:55] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:122cc6fc
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:029e1de0:start=1533346142846456894,finish=1533346142855326628,duration=8869734
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:033c5d48
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1a066b50
travis_time:start:1a066b50
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:15a6aec8
$ dmesg | grep -i kill

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

Can you add a suggestion to replace the tuple with an underscore, or a name which is the underscore concatenate of the tuple idents?

Great improvements I'm just worried about interactions with bad code being edited (like fn foo(u8 x: usize).

estebank · 2018-08-04T03:10:22Z

src/libsyntax/parse/parser.rs

+                    err.cancel();
+                    // Recover from attempting to parse the argument as a pattern. This means
+                    // the type is alone, with no name, e.g. `fn foo(u32)`.
+                    mem::replace(self, parser_snapshot_before_pat);


Can this be checked without rewinding state?proactively looking if a path could be parsed? I'm not against the rewind method in exceptional cases, but it should be a tool of last resort.

I had also wanted to avoid this, but I wasn't quite sure how to go about it. The problem I had was that parse_pat and parse_ty can succeed on the same strings. Ideally we'd parse_pat (which should be a superset of parse_ty), try to parse a : and then convert the pattern to a type if it failed, because we effectively have all the information we need at that stage. Is there any way to reinterpret a pattern as a type, or similar? (Or perhaps there's a simpler method I'm overlooking.)

I'm away from a computer now, but I'd think you could inspect the successfully parsed Pat to see if it is a single typed argument.

That seems plausible. I'll give it a go soon.

Rewinding is rare, so it's not very useful to optimize for that, but doing the state saving for every single fn parameter in traits doesn't look good.
I think doing minimal lookahead and avoiding state saving for the common case ident: Type would be enough.

Okay, I'll try that.

petrochenkov · 2018-08-05T15:03:07Z

src/libsyntax/parse/parser.rs

-            pat,
-            id: ast::DUMMY_NODE_ID,
-        })
+        let is_named_argument = self.is_named_argument();


Could you move this line into the Err(mut err) clause where it's needed?
(In the Ok(..) case it return nonsensical result anyway.)

Yeah, not sure why I left it out there...

petrochenkov · 2018-08-05T15:05:46Z

src/librustc_passes/ast_validation.rs

+                                    let mut err = struct_span_err!(self.session, span, E0642,
+                                        "patterns aren't allowed in methods without bodies");
+                                    err.span_suggestion(span,
+                                        "use an underscore to ignore the name", "_".to_owned());


What's the point of this recommendation?
If pattern doesn't work, then it's much more likely that the user will want some name describing the pattern as a whole, rather than no name at all.

It was a response to #53051 (review). This suggestion only appears somewhere where the name is irrelevant and can't be used anywhere else anyway, so a name is unnecessary. I considered converting the pattern somehow, but I couldn't find an existing method to do something similar, and it seemed like more effort than was worthwhile.

Synthesizing a meaningful name from code like fn foo((x, y): (i32, i32)); can be impossible to get right (should it suggest x_y? x_and_y? something else?), which is why I suggested using _, but the wording could be changed slightly to give this argument a name or use an underscore to ignore it, instead of a tuple pattern.

petrochenkov · 2018-08-05T15:54:45Z

Wait a second, this PR has language implications as well.

Arbitrary patterns are now accepted in trait methods with body, a feature that we can't provide without backtracking in parser.

trait Tr {
    fn f((a, b): (u8, u8)) {} // OK
}

This PR needs to enforce the restrictions that previously existed on all trait methods, with or without body.

varkor · 2018-08-05T16:10:15Z

Arbitrary patterns are now accepted in trait methods with body

Oops, is that the case? I didn't realise it would have that implication. There should probably be a test for that. I'll fix that and add a test.

estebank

Couple of smaller nitpicks, otherwise lgtm. Leaving @petrochenkov for final r.

estebank · 2018-08-06T20:13:07Z

src/libsyntax/parse/parser.rs

+        // If we see `ident :`, then we know that the argument is just of the
+        // form `type`, which means we won't need to recover from parsing a
+        // pattern and so we don't need to store a parser snapshot.
+        let parser_snapshot_before_pat = if


Creative :)

estebank · 2018-08-06T20:13:09Z

src/librustc_passes/diagnostics.rs

+                                // in methods without bodies
+}
+```
+"##,


Last nitpick, could you expand the documentation to show the "fixed" code, using maybe x_and_y as argument name?

estebank · 2018-08-06T20:15:08Z

src/test/ui/E0642.stderr

+   |
+LL |     fn bar((x, y): (i32, i32)) {} //~ ERROR patterns aren't allowed in trait methods
+   |            ^^^^^^
+help: give this argument a name or use an underscore to ignore it, instead of a tuple pattern


reword to "give this argument a name or use an underscore to ignore it instead of using a tuple pattern"

petrochenkov · 2018-08-06T22:08:00Z

src/librustc_passes/ast_validation.rs

-                            });
-                        }
+                            } else {
+                                let mut err = struct_span_err!(self.session, span, E0642,


Ok, now this is a breaking change 😄
Some limited set of pattern (defined by fn is_named_argument) was previously accepted by parser in methods with body.
Something like

trait Tr { fn f(&ident: Type) {} // OK fn g(&&ident: Type) {} // OK fn h(mut ident: Type) {} // OK fn k($pat: Type) {} // OK }

Note, that this compatibility check needs to happen in parser due to $pat that can look like an arbitrary complex pattern in AST validation, but still needs to be accepted.

Oh no! I thought I accounted for that with the branches, but it turns out not! (Why did it even pass? Do we not have any tests for that?)

So will I need to change parser.rs to fix this, or can I just move the check for the body?

I don't think AST validation needs any changes (it already does things right), all the relevant stuff is in the parser.
It would probably be better to:

Check for self.is_named_argument() and parse in the old way if it's true.

Otherwise, parse PAT: TYPE with state saving and immediately report a non-fatal error in case of success.

Otherwise, backtrack and parse TYPE.

If AST validation reports an unwanted duplicate error, then patterns could be converted into _ in the parser.

petrochenkov · 2018-08-09T00:10:56Z

src/librustc_passes/ast_validation.rs

+                                    "patterns aren't allowed in trait methods");
+                                let suggestion = "give this argument a name or use an \
+                                                  underscore to ignore it instead of using a \
+                                                  tuple pattern";


The mention of tuple pattern is overly specific, I'd remove the "instead" part, or used "instead of using a pattern"

varkor · 2018-08-09T22:25:02Z

Okay, the errors are now emitted during parsing instead, as per @petrochenkov's suggestion in #53051 (comment).

estebank · 2018-08-09T23:20:15Z

src/libsyntax/parse/parser.rs

+            // pattern and so we don't need to store a parser snapshot.
+            let parser_snapshot_before_pat = if
+                self.look_ahead(1, |t| t.is_ident()) &&
+                self.look_ahead(2, |t| t == &token::Colon) {


This would still store a copy of the parser for fn foo(&mut x) {} and fn bar(&self), right?

self is methods is treated separately outside of parse_arg_general, so it's not affected.

petrochenkov · 2018-08-11T14:20:31Z

src/librustc_passes/ast_validation.rs

@@ -342,10 +342,10 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
                                    self.session.buffer_lint(
                                        lint::builtin::PATTERNS_IN_FNS_WITHOUT_BODY,
                                        trait_item.id, span,
-                                        "patterns aren't allowed in methods without bodies");
+                                        "patterns aren't allowed in trait methods");


Patterns are allowed in trait methods with bodies, the old message was correct.

petrochenkov · 2018-08-11T14:21:13Z

Could you add examples from #53051 (comment) as a test for catching future regressions?

petrochenkov · 2018-08-11T14:26:36Z

src/libsyntax/parse/parser.rs

-            });
-            (pat, ty)
+
+            // If we see `ident :`, then we know that the argument is not just of the


ident : no longer needs to be treated specially because it falls under self.is_named_argument() above.

In other words, we are already in cold code here and can make the snapshot unconditionally.

petrochenkov · 2018-08-11T14:30:29Z

LGTM, beside the few remaining comments above.

The error and check for this already existed, but the parser didn't try to parse trait method arguments as patterns, so the error was never emitted. This surfaces the error, so we get better errors than simple parse errors.

petrochenkov · 2018-08-12T10:16:54Z

@bors r+

bors · 2018-08-12T10:16:55Z

📌 Commit 5c814e2 has been approved by petrochenkov

bors · 2018-08-12T10:16:55Z

🌲 The tree is currently closed for pull requests below priority 10, this pull request will be tested once the tree is reopened

…ts-error, r=petrochenkov Emit error for pattern arguments in trait methods The error and check for this already existed, but the parser didn't try to parse trait method arguments as patterns, so the error was never emitted. This surfaces the error, so we get better errors than simple parse errors. This improves the error message described in rust-lang#53046.

bors · 2018-08-13T02:28:21Z

⌛ Testing commit 5c814e2 with merge ab93561...

@petrochenkov

…=petrochenkov Emit error for pattern arguments in trait methods The error and check for this already existed, but the parser didn't try to parse trait method arguments as patterns, so the error was never emitted. This surfaces the error, so we get better errors than simple parse errors. This improves the error message described in #53046. r? @petrochenkov

bors · 2018-08-13T04:53:31Z

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

rust-highfive assigned petrochenkov Aug 4, 2018

varkor force-pushed the trait-method-pattern-arguments-error branch from 288699e to 934bca5 Compare August 4, 2018 01:22

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2018

varkor force-pushed the trait-method-pattern-arguments-error branch from 934bca5 to 023a714 Compare August 4, 2018 01:23

rust-lang deleted a comment from rust-highfive Aug 4, 2018

estebank reviewed Aug 4, 2018

View reviewed changes

varkor force-pushed the trait-method-pattern-arguments-error branch from 023a714 to d35252f Compare August 4, 2018 10:16

petrochenkov reviewed Aug 5, 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 Aug 5, 2018

estebank approved these changes Aug 6, 2018

View reviewed changes

petrochenkov reviewed Aug 6, 2018

View reviewed changes

petrochenkov reviewed Aug 9, 2018

View reviewed changes

varkor force-pushed the trait-method-pattern-arguments-error branch from 3d6eba1 to 96b34c1 Compare August 9, 2018 22:23

varkor force-pushed the trait-method-pattern-arguments-error branch from 96b34c1 to afe415d Compare August 9, 2018 22:58

estebank reviewed Aug 9, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

petrochenkov reviewed Aug 11, 2018

View reviewed changes

varkor added 4 commits August 11, 2018 21:08

Emit error for pattern arguments in trait methods

90a6954

The error and check for this already existed, but the parser didn't try to parse trait method arguments as patterns, so the error was never emitted. This surfaces the error, so we get better errors than simple parse errors.

Suggest replacing patterns with underscores

b05f0be

Fix handling of trait methods with bodies and improve efficiency

235905c

Improve diagnostics

a478cd4

varkor added 3 commits August 11, 2018 21:08

Emit an error during parsing

e4c3b49

Add E0642 to parser error

49e9c5f

Clean up and add extra tests

5c814e2

varkor force-pushed the trait-method-pattern-arguments-error branch from bb12fb0 to 5c814e2 Compare August 11, 2018 20:26

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 Aug 12, 2018

kennytm mentioned this pull request Aug 12, 2018

Rollup of 23 pull requests #53252

Closed

bors merged commit 5c814e2 into rust-lang:master Aug 13, 2018

pnkfelix mentioned this pull request Oct 8, 2018

[1.30 beta] Test suite of the jemalloc-ctl crate is failing #54478

Closed

ivanbakel mentioned this pull request Oct 13, 2018

Destructuring arguments in trait methods with bodies fails with a syntax error #55036

Closed

Emit error for pattern arguments in trait methods #53051

Emit error for pattern arguments in trait methods #53051

Conversation

varkor commented Aug 4, 2018

rust-highfive commented Aug 4, 2018

estebank 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

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

petrochenkov commented Aug 5, 2018

varkor commented Aug 5, 2018

estebank 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

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

varkor commented Aug 9, 2018

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

petrochenkov commented Aug 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Aug 11, 2018

petrochenkov commented Aug 12, 2018

bors commented Aug 12, 2018

bors commented Aug 12, 2018

bors commented Aug 13, 2018

bors commented Aug 13, 2018