Merge visitors in AST validation by Zoxc · Pull Request #57730 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation12 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.is_impl_trait_banned = old_is_impl_trait_banned;
}

fn with_impl_trait(&mut self, outer_impl_trait: Option, f: F)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use mem::replace?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use mem::replace here also?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM.

📌 Commit a5d4aed has been approved by cramertj

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

Centril added a commit to Centril/rust that referenced this pull request

…ertj

Merge visitors in AST validation

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

Centril added a commit to Centril/rust that referenced this pull request

…ertj

Merge visitors in AST validation

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

Centril added a commit to Centril/rust that referenced this pull request

…ertj

Merge visitors in AST validation

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

bors added a commit that referenced this pull request

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:

Zoxc deleted the combined-ast-validator branch

January 24, 2019 08:38

Zoxc added a commit to Zoxc/rust that referenced this pull request

Zoxc mentioned this pull request

Centril added a commit to Centril/rust that referenced this pull request

kennytm added a commit to kennytm/rust that referenced this pull request

bors added a commit that referenced this pull request

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)

bors added a commit that referenced this pull request

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)

bors added a commit that referenced this pull request

…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.

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.