Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

passes: prohibit invalid attrs on generic params #79073

Merged
merged 1 commit into from
Dec 19, 2020

Conversation

davidtwco
Copy link
Member

Fixes #78957.

This PR modifies the check_attr pass so that attribute placement on generic parameters is checked for validity.

r? @lcnr

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 15, 2020
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

LGTM.

Similar to #77015 this is once again a breaking change.

An example is

pub struct Foo<#[inline] T>(T);
pub struct Bar<#[cold] T>(T);
pub struct Baz<#[repr(C)]T>(T);

which previously compiled with

warning: unused attribute
 --> src/lib.rs:6:16
  |
6 | pub struct Baz<#[repr(C)]T>(T);
  |                ^^^^^^^^^^
  |
  = note: `#[warn(unused_attributes)]` on by default

warning: 1 warning emitted

but now errors with "attribute should be applied to ..."

I expect that we can probably have a quick crater run as the queue is nearly empty rn and then just merge this as is.

src/test/ui/issues/issue-78957.rs Show resolved Hide resolved
@lcnr lcnr added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 15, 2020
|
LL | #![deny(unused_attributes)]
| ^^^^^^^^^^^^^^^^^
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
Copy link
Contributor

Choose a reason for hiding this comment

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

So it seems like we always just emit a future compat lint if cold is applied incorrectly 🤔

It would be nice to increase that lint to deny by default. Don't know how much this would break though

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave this for a follow-up.

@lcnr lcnr added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2020
@jyn514 jyn514 added the A-attributes Area: #[attributes(..)] label Nov 16, 2020
@varkor varkor added A-const-generics Area: const generics (parameters and arguments) F-const_generics `#![feature(const_generics)]` labels Nov 17, 2020
@Mark-Simulacrum
Copy link
Member

@bors try for crater

@bors
Copy link
Contributor

bors commented Nov 17, 2020

⌛ Trying commit 0781e340faba9cf00de9f7f503c7793f2ed86e62 with merge 6f12b8fe67b4eed2e838dab803ed9493b9d62c0f...

@nikomatsakis
Copy link
Contributor

Please re-nominate when crater results are available!

@bors
Copy link
Contributor

bors commented Nov 17, 2020

☀️ Try build successful - checks-actions
Build commit: 6f12b8fe67b4eed2e838dab803ed9493b9d62c0f (6f12b8fe67b4eed2e838dab803ed9493b9d62c0f)

@nikomatsakis
Copy link
Contributor

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-79073 created and queued.
🤖 Automatically detected try build 6f12b8fe67b4eed2e838dab803ed9493b9d62c0f
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 17, 2020
@craterbot
Copy link
Collaborator

🚧 Experiment pr-79073 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-79073 is completed!
📊 6 regressed and 4 fixed (130845 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Nov 25, 2020
@lcnr
Copy link
Contributor

lcnr commented Nov 25, 2020

Looks like there are no regressions, renominated

This commit modifies the `check_attr` pass so that attribute placement
on generic parameters is checked for validity.

Signed-off-by: David Wood <david@davidtw.co>
@pnkfelix
Copy link
Member

pnkfelix commented Dec 1, 2020

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Dec 1, 2020

Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Dec 1, 2020
@rfcbot
Copy link

rfcbot commented Dec 8, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 18, 2020
@rfcbot
Copy link

rfcbot commented Dec 18, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@lcnr
Copy link
Contributor

lcnr commented Dec 18, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Dec 18, 2020

📌 Commit 75eb72c has been approved by lcnr

@bors 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 Dec 18, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 19, 2020
…ttrs, r=lcnr

passes: prohibit invalid attrs on generic params

Fixes rust-lang#78957.

This PR modifies the `check_attr` pass so that attribute placement on generic parameters is checked for validity.

r? `@lcnr`
@bors
Copy link
Contributor

bors commented Dec 19, 2020

⌛ Testing commit 75eb72c with merge 3d9ada6...

@bors
Copy link
Contributor

bors commented Dec 19, 2020

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 3d9ada6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 19, 2020
@bors bors merged commit 3d9ada6 into rust-lang:master Dec 19, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 19, 2020
@davidtwco davidtwco deleted the issue-78957-const-param-attrs branch December 19, 2020 09:01
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 27, 2020
…ination, r=varkor

stabilize `#![feature(min_const_generics)]` in 1.51

*A new Kind*
*A Sort long Prophesized*
*Once Fragile, now Eternal*

blocked on rust-lang#79073.

# Stabilization report

This is the stabilization report for `#![feature(min_const_generics)]` (tracking issue rust-lang#74878), a subset of `#![feature(const_generics)]` (tracking issue rust-lang#44580), based on rust-lang/rfcs#2000.

The [version target](https://forge.rust-lang.org/#current-release-versions) is ~~1.50 (2020-12-31 => beta, 2021-02-11 => stable)~~ 1.51 (2021-02-111 => beta, 2021-03-25 => stable).

This report is a collaborative effort of `@varkor,` `@shepmaster` and `@lcnr.`

## Summary

It is currently possible to parameterize functions, type aliases, types, traits and implementations by types and lifetimes.
With `#![feature(min_const_generics)]`, it becomes possible, in addition, to parameterize these by constants.

This is done using the syntax `const IDENT: Type` in the parameter listing. Unlike full const generics, `min_const_generics` is limited to parameterization by integers, and constants of type `char` or `bool`.

We already use `#![feature(min_const_generics)]` on stable to implement many common traits for arrays. See [the documentation](https://doc.rust-lang.org/nightly/std/primitive.array.html) for specific examples.

Generic const arguments, for now, are not permitted to involve computations depending on generic parameters. This means that const parameters may only be instantiated using either:

1. const expressions that do not depend on any generic parameters, e.g. `{ foo() + 1 }`, where `foo` is a `const fn`
1. standalone const parameters, e.g. `{N}`

### Example

```rust
#![feature(min_const_generics)]

trait Foo<const N: usize> {
    fn method<const M: usize>(&mut self, arr: [[u8; M]; N]);
}

struct Bar<T, const N: usize> {
    inner: [T; N],
}

impl<const N: usize> Foo<N> for Bar<u8, N> {
    fn method<const M: usize>(&mut self, arr: [[u8; M]; N]) {
        for (elem, s) in self.inner.iter_mut().zip(arr.iter()) {
            for &x in s {
                *elem &= x;
            }
        }
    }
}

fn function<const N: u16>() -> u16 {
    // Const parameters can be used freely inside of functions.
    (N + 1) / 2 * N
}

fn main() {
    let mut bar = Bar { inner: [0xff; 3] };
    // This infers the value of `M` from the type of the function argument.
    bar.method([[0b11_00, 0b01_00], [0b00_11, 0b00_01], [0b11_00, 0b00_11]]);
    assert_eq!(bar.inner, [0b01_00, 0b00_01, 0b00_00]);

    // You can also explicitly specify the value of `N`.
    assert_eq!(function::<17>(), 153);
}
```

## Motivation

Rust has the built-in array type, which is parametric over a constant. Without const generics, this type can be quite cumbersome to use as it is not possible to generically implement a trait for arrays of different lengths. For example, this meant that, for a long time, the standard library only contained trait implementations for arrays up to a length of 32. This restriction has since been lifted through the use of const generics.

Const parameters allow users to naturally specify variants of a generic type which are more naturally parameterized by values, rather than by types. For example, using const generics, many of the uses of the crate [typenum](https://crates.io/crates/typenum) may now be replaced with const parameters, improving compilation time as well as code readability and diagnostics.

The subset described by `min_const_generics` is self-contained, but extensive enough to help with the most frequent issues: implementing traits for arrays and using arbitrarily-sized arrays inside of other types. Furthermore, it extends naturally to full `const_generics` once the remaining design and implementation questions have been resolved.

## In-depth feature description

### Declaring const parameters

*Const parameters* are allowed in all places where types and lifetimes are supported. They use the syntax `const IDENT: Type`. Currently, const parameters must be declared after lifetime and type parameters. Their scope is equal to the scope of other generic parameters. They live in the value namespace.

`Type` must be one of `u8`, `u16`, `u32`, `u64`, `u128`, `usize`, `i8`, `i16`, `i32`, `i64`, `i128`, `isize`, `char` and `bool`. This restriction is implemented in two places:

1. during name resolution, where we forbid generic parameters
1. during well-formedness checking, where we only allow the types listed above

The updated syntax of parameter listings is:

```
GenericParams:
    (OuterAttr* LifetimeParam),* (OuterAttr* TypeParam),* (OuterAttr* ConstParam),*

OuterAttr: '#[' ... ']'
LifetimeParam: ...
TypeParam: ...
ConstParam: 'const' IDENT ':' Type
```

Unlike type and lifetime parameters, const parameters of types can be used without being mentioned inside of a parameterized type because const parameters do not have issues concerning variance. This means that the following types are allowed:

```rust
struct Foo<const N: usize>;
enum Bar<const M: usize> { A, B }
```

### Const arguments

Const parameters are instantiated using *const arguments*. Any concrete const expression or const parameter as a standalone argument can be used. When applying an expression as const parameter, most expressions must be contained within a block, with two exceptions:

1. literals and single-segment path expressions
1. array lengths

This syntactic restriction is necessary to avoid ambiguity, or requiring infinite lookahead when parsing an expression as a generic argument.

In the cases where a generic argument could be resolved as either a type or const argument, we always interpret it as a type. This causes the following test to fail:

```rust
type N = u32;
struct Foo<const N: usize>;
fn foo<const N: usize>() -> Foo<N> { todo!() } // ERR
```

To circumvent this, the user may wrap the const parameter with braces, at which point it is unambiguously accepted.

```rust
type N = u32;
struct Foo<const N: usize>;
fn bar<const N: usize>() -> Foo<{ N }> { todo!() } // ok
```

Operations depending on generic parameters are **not** allowed, which is enforced during well-formedness checking. Allowing generic unevaluated constants would require a way to check if they would always evaluate successfully to prevent errors that are not caught at declaration time. This ability forms part of `#![feature(const_evaluatable_checked)]`, which is not yet being stabilised.

Since we are not yet stabilizing `#![feature(lazy_normalization_consts)]`, we must not supply the parent generics to anonymous constants except for repeat expressions. Doing so can cause cycle errors for arrays used in `where`-bounds. Not supplying the parent generics can however lead to ICEs occurring before well-formedness checking when trying to use a generic parameter. See rust-lang#56445 for details.

Since we expect cases like this to occur more frequently once `min_const_generics` is stabilized, we have chosen to forbid generic parameters in anonymous constants during name resolution. While this changes the ICE in the situation above to an ordinary error, this is theoretically a breaking change, as early-bound lifetimes were previously permitted in repeat expressions but now are disallowed, causing the following snippet to break:

```rust
fn late_bound<'a>() {
    let _ = [0; {
        let _: &'a (); // ICE ==> ERR
        3
    }];
}

fn early_bound<'a>() where &'a (): Sized {
    let _ = [0; {
        let _: &'a (); // ok ==> ERR
        3
    }];
}
```

### Using const parameters

Const parameters can be used almost everywhere ordinary constants are allowed, except that they may not be used in the construction of consts, statics, functions, or types inside a function body and are subject to the generic argument restrictions mentioned above.

Expressions containing const parameters are eligible for promotion:

```rust
fn test<const N: usize>() -> &'static usize {
    &(3 + N)
}
```

### Symbol mangling

See the [Rust symbol name mangling RFC](https://rust-lang.github.io/rfcs/2603-rust-symbol-name-mangling-v0.html) for an overview. Generic const parameters take the form `K[type][value]` when the value is known, or `Kp` where the value is not known, where:
- `[type]` is any integral type, `bool`, or `char`.
- `[value]` is the unsigned hex value for integers, preceded by `n` when negative; is `0` or `1` for `bool`; is the hex value for `char`.

### Exhaustiveness checking

We do not check the exhaustiveness of impls, meaning that the following example does **not** compile:

```rust
struct Foo<const B: bool>;
trait Bar {}
impl Bar for Foo<true> {}
impl Bar for Foo<false> {}

fn needs_bar(_: impl Bar) {}
fn generic<const B: bool>() {
    let v = Foo::<B>;
    needs_bar(v);
}
```

### Type inference

The value of const parameters can be inferred during typeck. One interesting case is the length of generic arrays, which can also be inferred from patterns (implemented in rust-lang#70562). Practical usage of this can be seen in rust-lang#76825.

### Equality of constants

`#![feature(min_const_generics)]` only permits generic parameters to be used as standalone generic arguments. We compare two parameters to be equal if they are literally the same generic parameter.

### Associated constants

Associated constants can use const parameters without restriction, see rust-lang#79135 (comment) for more details.

## Future work

As this is a limited subset of rust-lang/rfcs#2000, there are quite a few extensions we will be looking into next.

### Lazy normalization of constants

Stabilizing `#![feature(lazy_normalization_consts)]` (tracking issue rust-lang#72219) will remove some special cases that are currently necessary for `min_const_generics`, and unblocks operations on const parameters.

### Relaxing ordering requirements between const and type parameters

We currently restrict the order of generic parameters so that types must come before consts. We could relax this, as is currently done with `const_generics`. Without this it is not possible to use both type defaults and const parameters at the same time.

Unrestricting the order will require us to improve some diagnostics that expect there to be a strict order between type and const parameters.

### Allowing more parameter types

We would like to support const parameters of more types, especially`&str` and user-defined types. Both are blocked on [valtrees]. There are also open questions regarding the design of `structural_match` concerning the latter. Supporting generic const parameter types such as `struct Foo<T, const N: T>` will be a lot harder and is unlikely to be implemented in the near future.

### Default values of const parameters

We do not yet support default values for const parameters. There is work in progress to enable this on nightly (see rust-lang#75384).

### Generic const operations

With `#![feature(min_const_generics)]`, only concrete const expressions and parameters as standalone arguments are allowed in types and repeat expressions. However, supporting generic const operations, such as `N + 1` or `std::mem::size_of::<T>()` is highly desirable. This feature is in early development under `#![feature(const_evaluatable_checked)]`.

## Implementation history

Many people have contributed to the design and implementation of const generics over the last three years. See rust-lang#44580 (comment) for a summary. Once again thank you to everybody who helped out here!

[valtrees]: rust-lang#72396

---

r? `@varkor`
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: #[attributes(..)] A-const-generics Area: const generics (parameters and arguments) disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-const_generics `#![feature(const_generics)]` finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

attributes on const params do not cause an error