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.

Sign up for GitHub

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

Jump to bottom

Tidy feature: deny commits with a tracking issue of 0 #41260

Closed
clarfonthey opened this issue Apr 12, 2017 · 5 comments · Fixed by #67480
Closed

Tidy feature: deny commits with a tracking issue of 0 #41260

clarfonthey opened this issue Apr 12, 2017 · 5 comments · Fixed by #67480
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@clarfonthey
Copy link
Contributor

clarfonthey commented Apr 12, 2017 •
edited
Loading

A reasonable number of commits get pushed that forget a tracking issue, so, I think that this would be useful to have. Additionally, it'd be nice if any #[unstable] attributes got checked to make sure that the tracking issues are all the same.

This should also check links in the unstable book.
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Apr 14, 2017 •
edited
Loading

I'm against it. A lot of commits/improvements don't have an issue because it has been discussed outside github.

@Mark-Simulacrum Mark-Simulacrum added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jun 20, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 27, 2017
@clarfonthey clarfonthey mentioned this issue Apr 15, 2018
Merged
@varkor
Copy link
Member

varkor commented Apr 22, 2019

As discussed in #49985 (comment), the way forward here is making the issue field optional. This should be easy to fix. You can probably just remove this branch:

rust/src/libsyntax/attr/builtin.rs

Lines 363 to 366 in 6d59933

_ => {
span_err!(diagnostic, attr.span, E0547, "missing 'issue'");
continue
}

and remove any existing issue = "0" fields.

@varkor varkor added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Apr 22, 2019
@Julian-Wollersberger
Copy link
Contributor

I have looked into this and the easiest way to do it is to internally use 0 as the issue number if it wasn't given.
This feels a bit hacky. Should I try to change the type in StabilityLevel::Unstable from issue: i32 to issue: Option<i32> instead?

@varkor
Copy link
Member

varkor commented Apr 27, 2019

Should I try to change the type in StabilityLevel::Unstable from issue: i32 to issue: Option<i32> instead?

Yes, that's right. 0 should not be used to encode an issue anywhere; we should be using Option instead.

@Julian-Wollersberger
Copy link
Contributor

Ok, the types were changed to issue: Option<i32> in a few places and I added a basic ui test. It turned out that issue fields were already Option<i32> in most places.

Tomorrow I'll try to fix the 79 #[unstable(... , issue = "0")] I found. (And make a proper warning)

Julian-Wollersberger added a commit to Julian-Wollersberger/rust that referenced this issue May 15, 2019
@Julian-Wollersberger
Began changing the type in StabilityLevel::Unstable from `issue: i3…
547e46a 
…2` to `issue: Option<i32>`

rust-lang#41260
Closed
@rossmacarthur rossmacarthur mentioned this issue Nov 11, 2019
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this issue Nov 12, 2019
@JohnTitor
Rollup merge of rust-lang#66299 - rossmacarthur:fix-41260-avoid-issue…
4134a4a 
…-0, r=varkor

support issue = "none" in unstable attributes

This works towards fixing rust-lang#41260.

This PR allows the use of `issue = "none"` in unstable attributes and makes changes to internally store the issue number as an `Option<NonZeroU32>`. For example:

```rust
#[unstable(feature = "unstable_test_feature", issue = "none")]
fn unstable_issue_none() {}
```

It was not made optional because feedback seen here rust-lang#60860 suggested that people might forget the issue field if it was optional.

I could not remove the current uses of `issue = "0"` (of which there are a lot) because the stage 0 compiler expects the old syntax. Once this is available in the stage 0 compiler we can replace all uses of `"0"` with `"none"` and no longer allow `"0"`. This is my first time contributing, so I'm not sure what the protocol is with two-part things like this, so some guidance would be appreciated.

r? @varkor
This was referenced Dec 21, 2019
Merged
Closed
Centril added a commit to Centril/rust that referenced this issue Dec 22, 2019
@Centril
Rollup merge of rust-lang#67480 - rossmacarthur:fix-41260-avoid-issue…
cdb8cd3 
…-0-part-2, r=Centril

Require issue = "none" over issue = "0" in unstable attributes

These changes make the use of `issue = "none"` required in unstable attributes throughout the compiler.

Notes:
- rust-lang#66299 is now in beta so `issue = "none"` is accepted.
- The `tidy` tool now fails on `issue = "0"`.
- Tests that used `issue = "0"` were changed to use `issue = "none"`, except for _one_ that asserts `issue = "0"` can still be used.
- The compiler still allows `issue = "0"` because some submodules require it, this could be disallowed once these are updated.

Resolves rust-lang#41260

r? @varkor
@bors bors closed this as completed in eaeb113 Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
No milestone
5 participants
@GuillaumeGomez @varkor @Mark-Simulacrum @clarfonthey @Julian-Wollersberger