Prohibit parenthesized params in more types.

[qnighy]

Prohibit parenthesized parameters in primitive types, type parameters, Self, etc.

Fixes #32995.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[TimNN]

Hi @qnighy, thanks for the PR! We'll regularly check in to make sure that @arielb1 or someone else from the team reviews this soon.

[qnighy]

@TimNN thanks for response too! So now I can just wait for review, right? I'm just confused about the meaning of "S-waiting-on-author".

[arielb1]

Sorry for being late on review.

There's another case here: https://github.com/qnighy/rust/blob/63ecd6aa0f2c8a6807528d8bdf5eb30211b3fcd4/src/librustc_typeck/astconv.rs#L1395

Example test case:

use std::fmt::Display;

fn main() {
    let s: Box<Display+std<Display>::marker::Sync()> = Box::new(2);
}

Basically every time we access segments we want to make sure they have the correct form.

[qnighy]

Thanks for reviewing, and I found another case which leads to ICE.

macro_rules! pathexpr {
    ($p:path) => ($p);
}
fn main() {
    println!("{}", pathexpr!(std::str::from_utf8())(b"Hello").unwrap());
}

error: internal compiler error: src/librustc_typeck/check/mod.rs:4498: parenthesized parameters cannot appear in ExprPath
 --> test.rs:2:19
  |
2 |     ($p:path) => ($p);
  |                   ^^
...
5 |     println!("{}", pathexpr!(std::str::from_utf8())(b"Hello").unwrap());
  |                    -------------------------------- in this macro invocation

[qnighy]

In summary, we'll have to additionally deal with

• ICE for () appearing in expression-like path
• () in module path for bounds, trait objects and impls
• Send/Sync bound for trait objects

macro_rules! pathexpr {
    (($($x:tt)*), $p:path, ($($y:tt)*)) => ($($x)* $p $($y)*);
}

mod m1 {
    pub struct A {}
    pub struct B();
    pub struct C;
    pub trait Marker {}
}

trait Tr {
    type A;
    type B;
    type C;
    fn f();
}

struct D;
impl Tr for D {
    type A = m1::A;
    type B = m1::B;
    type C = m1::C;
    fn f() {}
}

// ItemUse
// pathexpr!((use), std::boxed()::Box, (;)); // ERROR from ast_validation

// Visibility
mod m2 {
    macro_rules! foo {
        ($p:path) => { pub(in $p) mod mm {} }
    }
    // foo!(m2()); // ERROR from ast_validation
}

fn main() {
    // QPath::Resolved in PatKind::Struct
    // let pathexpr!((), m1()::A , ({})) = m1::A {}; // ERROR from astconv
    // let pathexpr!((), m1::A() , ({})) = m1::A {}; // ERROR from astconv

    // QPath::Resolved in PatKind::TupleStruct
    // let pathexpr!((), m1()::B , (())) = m1::B(); // ERROR from astconv
    // let pathexpr!((), m1::B() , (())) = m1::B(); // ICE

    // QPath::Resolved in PatKind::Path
    // let pathexpr!((), m1()::C , ()) = m1::C; // ERROR from astconv
    // let pathexpr!((), m1::C() , ()) = m1::C; // ICE

    // QPath::Resolved in ExprPath
    // pathexpr!((), m1()::C, ()); // ERROR from astconv
    // pathexpr!((), m1::C(), ()); // ICE

    // QPath::Resolved in ExprStruct
    // pathexpr!((), m1()::A, ({})); // ERROR from astconv
    // pathexpr!((), m1::A(), ({})); // ERROR from astconv

    // QPath::Resolved in TyPath
    // let _ : m1()::C = m1::C; // ERROR from astconv
    // let _ : m1::C() = m1::C; // ERROR from astconv

    // QPath::TypeRelative in PatKind::Struct
    // let pathexpr!((), D::A(), ({})) = m1::A {}; // ERROR from astconv

    // QPath::TypeRelative in PatKind::TupleStruct
    // let pathexpr!((), D::B(), (())) = m1::B (); // resolution error

    // QPath::TypeRelative in PatKind::Path
    // let pathexpr!((), D::C, ()) = m1::C; // resolution error

    // QPath::TypeRelative in ExprPath
    // pathexpr!((), D::f(), ()); // ICE

    // QPath::TypeRelative in ExprStruct
    // pathexpr!((), D::A(), ({})); // ERROR from astconv

    // QPath::TypeRelative in TyPath
    // let _ : D::C() = m1::C; // ERROR from astconv

    // TraitTyParamBound
    { fn f<X: m1()::Marker>() {} }
    // { fn f<X: m1::Marker()>() {} } // type error

    // TyTraitObject
    let x : &m1()::Marker;
    // let x : &m1::Marker(); // type error
    let x : &(m1::Marker + Sync());

    // ItemDefaultImpl: omitted

    // ItemImpl
    impl m1()::Marker for u32 {}
    // impl m1::Marker() for i32 {} // type error
}

[qnighy]

Dealt with all path segments I found so far.

[arielb1]

@bors r+

[bors]

📌 Commit 2a20073 has been approved by arielb1

[bors]

⌛ Testing commit 2a20073 with merge a89314f...

[bors]

💔 Test failed - status-appveyor

[qnighy]

Oh, this is the first failure for my PR to rust-lang... It is a spurious failure, I suppose?

[petrochenkov]

Yeah, it's spurious, should be mitigated by #41996
@bors retry

[frewsxcv]

@bors r-

this hit an error on travis https://travis-ci.org/rust-lang/rust/jobs/232310581

[qnighy]

An existing compile-fail test hit a newly introduced error.

This time I ran all tests and it should be fixed.

[Mark-Simulacrum]

@bors r=arielb1

[bors]

📌 Commit e8137d7 has been approved by arielb1

[bors]

⌛ Testing commit e8137d7 with merge 0915d89...

[Mark-Simulacrum]

@bors r-
@bors retry

This needed a crater run, and I don't think that happened.

[qnighy]

@Mark-Simulacrum I suppose no. Is that what I can do by myself?

[brson]

I've started builds for a crater run

• crater Prohibit parenthesized params in more types. #41856
  • https://github.com/qnighy/rust
  • before bedd7da
  • https://tools.taskcluster.net/task-inspector/#NvXyrF1rS8WQ7R8aMPYFHA
  • after e8137d7
  • https://tools.taskcluster.net/task-inspector/#JtjGY6ImQJWLiH2E83IRuQ

[brson]

I've started the crate build.

[brson]

https://gist.github.com/anonymous/bf8df80700bed163982131ff15c3df3e

[nikomatsakis]

Regressions

• There are 27 root regressions
• There are 88 regressions

Root regressions, sorted by rank:

• libflate-0.1.6 (before) (after)
• twox-hash-1.1.0 (before) (after)
• encoding_rs-0.6.10 (before) (after)
• spidev-0.3.0 (before) (after)
• bitintr-0.1.18 (before) (after)
• widestring-0.2.2 (before) (after)
• netopt-0.1.3 (before) (after)
• odbc-sys-0.3.0 (before) (after)
• gpsd-0.1.2 (before) (after)
• haxonite-0.1.0 (before) (after)
• hyper_serde-0.6.2 (before) (after)
• iron-drain-0.1.2 (before) (after)
• collider-0.2.1 (before) (after)
• libalpm-utils-0.0.2 (before) (after)
• curse-0.1.0 (before) (after)
• diagnostics-0.1.0 (before) (after)
• pdb-0.1.3 (before) (after)
• pngeq-0.1.0 (before) (after)
• puzzle-solver-0.4.1 (before) (after)
• rls-analysis-0.4.0 (before) (after)
• rust_RIBBIT-0.0.1 (before) (after)
• spirv-0.1.0 (before) (after)
• split_by-0.2.0 (before) (after)
• ssh-0.1.4 (before) (after)
• unescape-0.1.0 (before) (after)
• webscale_plugin-0.1.0 (before) (after)
• xz-0.0.1 (before) (after)

[Mark-Simulacrum]

Actual root regressions. All look "intentional" though none seem needed for any reason.

• libflate-0.1.6 (before) (after) - as u16()
• twox-hash-1.1.0 (before) (after) - macro invocation, -> usize()
• encoding_rs-0.6.10 (before) (after) - -> usize()
• pdb-0.1.3 (before) (after) - -> usize()
• rls-analysis-0.4.0 (before) (after) - as isize()
• rust_RIBBIT-0.0.1 (before) (after) - -> u64()

[nikomatsakis]

Looks like these are legit -- I think we need a warning cycle. @qnighy, a rough description of the procedure for warning cycles is described here on forge.

[petrochenkov]

I can't believe people actually write this.
How does that happen? There should be some explanation.

[qnighy]

OK, I'll try it. I didn't expect this is actually exploited in several libraries.

[qnighy]

This PR now have two changes:

1. Prohibit parenthesized parameters () where angle-bracketed parameters <T> are already prohibited.
2. Prohibit parameters of any kind () (T) <T> where even <T> was not checked.

It seems that only the former one hit regressions. Is it sufficient to only prepare a warning cycle for the former one?

[qnighy]

Prepared the tracking issue for the warning cycle: #42238

[qnighy]

Added the warning cycle. I wonder if this is sufficient?

[shepmaster]

I can't believe people actually write this.
How does that happen? There should be some explanation.

@petrochenkov I can only speak for my own crate, but my guess is that I did some copy-pasta when implementing size_hint and count, and just ended up with some rogue parens that didn't hurt anything.

[arielb1]

Do we do another crater run then? r+ modulo that.

[arielb1]

Well, let's go

@bors r+

[bors]

📌 Commit 9999378 has been approved by arielb1

[bors]

⌛ Testing commit 9999378 with merge 03bed65...

[qnighy]

Could anyone tag the tracking issue 42292 #42238 with B-unstable and T-compiler?

[Mark-Simulacrum]

Added tags to #42238; the issue you referenced doesn't appear to be the right tracking issue.

[qnighy]

Thank you, that was just my mistake.

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 03bed65 to master...

[qnighy]

Thank you all for reviewing!