Merge visitors in AST validation #57730

Zoxc · 2019-01-18T11:37:04Z

Cuts runtime for AST validation on syntex_syntax from 31.5 ms to 17 ms.

rust-highfive · 2019-01-18T11:37:07Z

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

Centril · 2019-01-18T12:51:06Z

src/librustc_passes/ast_validation.rs

+
+    // Used to ban nested `impl Trait`, e.g., `impl Into<impl Debug>`.
+    // Nested `impl Trait` _is_ allowed in associated type position,
+    // e.g `impl Iterator<Item=impl Debug>`


Maybe also add a note about why we do this (cause we are not sure whether we want it to be higher rank: impl for<T: Debug> Into<T>, or rank-1).

I don't know why we do this. I just copied this comment. Feel free to suggest the exact change you'd like.

Centril · 2019-01-18T12:51:43Z

src/librustc_passes/ast_validation.rs

 }

 impl<'a> AstValidator<'a> {
+    fn with_banned_impl_trait<F>(&mut self, f: F)


use f: impl FnOnce(&mut Self) for increased readability?

Centril · 2019-01-18T12:52:42Z

src/librustc_passes/ast_validation.rs

+        self.is_impl_trait_banned = old_is_impl_trait_banned;
+    }
+
+    fn with_impl_trait<F>(&mut self, outer_impl_trait: Option<Span>, f: F)


Same.. f: impl FnOnce(&mut Self).

Centril · 2019-01-18T12:53:14Z

src/librustc_passes/ast_validation.rs

+    fn with_banned_impl_trait<F>(&mut self, f: F)
+        where F: FnOnce(&mut Self)
+    {
+        let old_is_impl_trait_banned = self.is_impl_trait_banned;


Use mem::replace?

Centril · 2019-01-18T12:53:44Z

src/librustc_passes/ast_validation.rs

+    fn with_impl_trait<F>(&mut self, outer_impl_trait: Option<Span>, f: F)
+        where F: FnOnce(&mut Self)
+    {
+        let old_outer_impl_trait = self.outer_impl_trait;


use mem::replace here also?

cramertj · 2019-01-18T19:01:38Z

r=me with @Centril's nits fixed.

Zoxc · 2019-01-19T03:34:02Z

src/librustc_passes/ast_validation.rs

-    fn visit_ty(&mut self, t: &'a Ty) {
-        match t.node {
-            TyKind::ImplTrait(..) => {
-                if self.is_banned {


This PR will give errors about nested impl traits being banned in path parameters, unlike the old code, which doesn't visit inside impl Trait. So we'd get 3 errors then, one for nested impl Trait and one for each instance of impl Trait in a path parameter.

cramertj · 2019-01-21T19:00:14Z

@bors r+

bors · 2019-01-21T19:00:15Z

📌 Commit a5d4aed has been approved by cramertj

…ertj Merge visitors in AST validation Cuts runtime for AST validation on `syntex_syntax` from 31.5 ms to 17 ms.

@ghost

Rollup of 11 pull requests Successful merges: - #57179 (Update std/lib.rs docs to reflect Rust 2018 usage) - #57730 (Merge visitors in AST validation) - #57779 (Recover from parse errors in literal struct fields and incorrect float literals) - #57793 (Explain type mismatch cause pointing to return type when it is `impl Trait`) - #57795 (Use structured suggestion in stead of notes) - #57817 (Add error for trailing angle brackets.) - #57834 (Stabilize Any::get_type_id and rename to type_id) - #57836 (Fix some cross crate existential type ICEs) - #57840 (Fix issue 57762) - #57844 (use port 80 for retrieving GPG key) - #57858 (Ignore line ending on older git versions) Failed merges: r? @ghost

@cramertj

Fix rust-lang#57730 cc rust-lang#57730 r? @cramertj

@cramertj

Fix rust-lang#57730 cc rust-lang#57730 r? @cramertj

Rollup of 19 pull requests Successful merges: - #57929 (Rustdoc remove old style files) - #57981 (Fix #57730) - #58074 (Stabilize slice_sort_by_cached_key) - #58196 (Add specific feature gate error for const-unstable features) - #58293 (Remove code for updating copyright years in generate-deriving-span-tests) - #58306 (Don't default on std crate when manipulating browser history) - #58359 (librustc_mir: use ? in impl_snapshot_for! macro) - #58395 (Instant::checked_duration_since) - #58429 (fix Box::into_unique effecitvely transmuting to a raw ptr) - #58433 (Update which libcore/liballoc tests Miri ignores, and document why) - #58438 (Use posix_spawn_file_actions_addchdir_np when possible) - #58440 (Whitelist the ARM v6 target-feature) - #58448 (rustdoc: mask `compiler_builtins` docs) - #58468 (split MaybeUninit into several features, expand docs a bit) - #58477 (Fix the syntax error in publish_toolstate.py) - #58479 (compile-pass test for #53606) - #58489 (Fix runtime error in generate-keyword-tests) - #58496 (Fix documentation for std::path::PathBuf::pop) - #58509 (Notify myself when Clippy toolstate changes)

Rollup of 19 pull requests Successful merges: - #57929 (Rustdoc remove old style files) - #57981 (Fix #57730) - #58074 (Stabilize slice_sort_by_cached_key) - #58196 (Add specific feature gate error for const-unstable features) - #58293 (Remove code for updating copyright years in generate-deriving-span-tests) - #58306 (Don't default on std crate when manipulating browser history) - #58359 (librustc_mir: use ? in impl_snapshot_for! macro) - #58395 (Instant::checked_duration_since) - #58429 (fix Box::into_unique effecitvely transmuting to a raw ptr) - #58433 (Update which libcore/liballoc tests Miri ignores, and document why) - #58438 (Use posix_spawn_file_actions_addchdir_np when possible) - #58440 (Whitelist the ARM v6 target-feature) - #58448 (rustdoc: mask `compiler_builtins` docs) - #58468 (split MaybeUninit into several features, expand docs a bit) - #58479 (compile-pass test for #53606) - #58489 (Fix runtime error in generate-keyword-tests) - #58496 (Fix documentation for std::path::PathBuf::pop) - #58509 (Notify myself when Clippy toolstate changes) - #58521 (Fix tracking issue for error iterators)

…mpl-trait, r=zoxc Warning period for detecting nested impl trait Here is some proposed code for making a warning period for the new checking of nested impl trait. It undoes some of the corrective effects of PR #57730, by using boolean flags to track parts of the analysis that were previously skipped prior to PRs #57730 and #57981 landing. Cc #57979

rust-highfive assigned cramertj Jan 18, 2019

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

Merge visitors in AST validation

13dc584

Zoxc force-pushed the combined-ast-validator branch from e3c2264 to 13dc584 Compare January 18, 2019 12:12

Centril reviewed Jan 18, 2019

View reviewed changes

Address some comments

a5d4aed

Zoxc commented Jan 19, 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 21, 2019

Centril added a commit to Centril/rust that referenced this pull request Jan 21, 2019

Rollup merge of rust-lang#57730 - Zoxc:combined-ast-validator, r=cram…

45e6a2b

…ertj Merge visitors in AST validation Cuts runtime for AST validation on `syntex_syntax` from 31.5 ms to 17 ms.

Centril mentioned this pull request Jan 21, 2019

Rollup of 8 pull requests #57823

Closed

Centril added a commit to Centril/rust that referenced this pull request Jan 23, 2019

Rollup merge of rust-lang#57730 - Zoxc:combined-ast-validator, r=cram…

8da08cf

…ertj Merge visitors in AST validation Cuts runtime for AST validation on `syntex_syntax` from 31.5 ms to 17 ms.

Centril mentioned this pull request Jan 23, 2019

Rollup of 8 pull requests #57850

Closed

Centril added a commit to Centril/rust that referenced this pull request Jan 23, 2019

Rollup merge of rust-lang#57730 - Zoxc:combined-ast-validator, r=cram…

b0ec43f

…ertj Merge visitors in AST validation Cuts runtime for AST validation on `syntex_syntax` from 31.5 ms to 17 ms.

Centril mentioned this pull request Jan 23, 2019

Rollup of 11 pull requests #57869

Merged

bors merged commit a5d4aed into rust-lang:master Jan 24, 2019

Zoxc deleted the combined-ast-validator branch January 24, 2019 08:38

jonas-schievink mentioned this pull request Jan 29, 2019

Regression from stable to nightly: nested impl trait is not allowed #57979

Closed

Zoxc added a commit to Zoxc/rust that referenced this pull request Jan 30, 2019

Fix rust-lang#57730

e6d5f25

Zoxc mentioned this pull request Jan 30, 2019

Fix #57730 #57981

Merged

Centril added a commit to Centril/rust that referenced this pull request Feb 14, 2019

Rollup merge of rust-lang#57981 - Zoxc:fix-57979, r=nikomatsakis

b9efe60

Fix rust-lang#57730 cc rust-lang#57730 r? @cramertj

Centril mentioned this pull request Feb 14, 2019

Rollup of 9 pull requests #58470

Closed

kennytm mentioned this pull request Feb 15, 2019

Rollup of 19 pull requests #58495

Merged

kennytm added a commit to kennytm/rust that referenced this pull request Feb 16, 2019

Rollup merge of rust-lang#57981 - Zoxc:fix-57979, r=nikomatsakis

84e88da

Fix rust-lang#57730 cc rust-lang#57730 r? @cramertj

pnkfelix mentioned this pull request Feb 21, 2019

Warning period for detecting nested impl trait #58608

Merged

pnkfelix mentioned this pull request Mar 8, 2019

Tracking issue for nested_impl_trait compatibility lint #59014

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge visitors in AST validation #57730

Merge visitors in AST validation #57730

Zoxc commented Jan 18, 2019

rust-highfive commented Jan 18, 2019

Centril Jan 18, 2019

Zoxc Jan 19, 2019

Centril Jan 18, 2019

Centril Jan 18, 2019

Centril Jan 18, 2019

Centril Jan 18, 2019

cramertj commented Jan 18, 2019

Zoxc Jan 19, 2019

cramertj Jan 21, 2019

cramertj commented Jan 21, 2019

bors commented Jan 21, 2019

Merge visitors in AST validation #57730

Merge visitors in AST validation #57730

Conversation

Zoxc commented Jan 18, 2019

rust-highfive commented Jan 18, 2019

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

cramertj commented Jan 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj commented Jan 21, 2019

bors commented Jan 21, 2019