Make `issue` field optional in `#[unstable]` to avoid `issue = "0"` #60860

Julian-Wollersberger · 2019-05-15T19:00:56Z

As requested, I made the issue field in #[unstable] optional.
Removing all the issue = "0" in libstd and libcore wasn't possible though, because the stage 0 compiler emits an error for the missing issue fields.
What's the best solution to do the cleanup? Maybe merging this PR, waiting for the next master to beta promotion and doing the cleanup in a follow-up PR?
The UI tests are cleaned up though.

I also wanted to emit a warning for issue = "0" but ran into the same stage 0 problem.
And how do you choose or omit the error number in a struct_span_warn!()?

Thank you for the mentoring, @varkor!

…2` to `issue: Option<i32>` rust-lang#41260

…o test them.

…asic ui test. It turned out that issue fields were already Option<i32> in most places. Added some temporary errors to find all the `#[unstable(... , issue = "0")]`

rust-highfive · 2019-05-15T19:01:00Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (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.

rust-highfive · 2019-05-15T19:18:58Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:18742968:start=1557946922106419963,finish=1557946925734907099,duration=3628487136
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---

[00:04:25] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:26] tidy error: /checkout/src/libsyntax/attr/builtin.rs:348: TODO is deprecated; use FIXME
[00:04:26] tidy error: /checkout/src/libsyntax/attr/builtin.rs:360: TODO is deprecated; use FIXME
[00:04:26] tidy error: /checkout/src/libsyntax/error_codes.rs:435: TODO is deprecated; use FIXME
[00:04:31] some tidy checks failed
[00:04:31] 
[00:04:31] 
[00:04:31] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:31] 
[00:04:31] 
[00:04:31] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:31] Build completed unsuccessfully in 0:01:10
[00:04:31] Build completed unsuccessfully in 0:01:10
[00:04:31] Makefile:67: recipe for target 'tidy' failed
[00:04:31] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:022f81aa
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed May 15 19:06:47 UTC 2019
---
travis_time:end:00694e00:start=1557947207892396626,finish=1557947207896841279,duration=4444653
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:083cf84a
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:2430b2e3
travis_time:start:2430b2e3
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0097edde
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

petrochenkov · 2019-05-15T20:01:20Z

This makes things worse, it's now much easier to forget the issue field.
issue = "0" is only used in tests, so making it optional makes those few tests nicer at cost of regressing all the remaining code.

If tidy check is wanted, it can ignore the test subdirectory.

varkor · 2019-05-15T22:04:23Z

issue = "0" is only used in tests, so making it optional makes those few tests nicer at cost of regressing all the remaining code.

issue = "0" is used not infrequently in libcore/libstd (though often for internal features). I do think accidentally forgetting an issue is a concern, though.

Maybe we could explicitly declare issueless features (e.g. issue = "none" or issue = "internal", etc.). This would be equivalent syntactically to issue = "0", but more semantically obvious. Using Option seems preferable than a magic number too.

hellow554 · 2019-05-16T06:12:23Z

src/librustc/middle/stability.rs

@@ -515,7 +515,7 @@ pub enum EvalResult {
    Deny {
        feature: Symbol,
        reason: Option<Symbol>,
-        issue: u32,
+        issue: Option<u32>,


What about using a Option<NonZeroU32>> instead? There's no valid issue #0, which means that we can try to save some space here.

It can't be done in this PR though, because the stage 0 compiler expects the old syntax and therefore the issue = "0" in stdlib and stdcore must remain until the changes made here get into stage 0.
(To be precise, x.py check emits errors and AFAIK Travis checks that.)
Also, there are other places where issues are represented as Option<u32> and even Some(0)

src/librustc/session/mod.rs

src/libsyntax/attr/builtin.rs

hellow554 · 2019-05-16T06:13:39Z

src/libsyntax/error_codes.rs

@@ -432,7 +432,7 @@ register_diagnostics! {
    E0544, // multiple stability levels
    E0545, // incorrect 'issue'
    E0546, // missing 'feature'
-    E0547, // missing 'issue'
+    E0547, // missing 'issue' //TODO I reused this one for the warning.


Is that a good idea? :/

No it's not, but I don't know how to properly do it. Can someone help me here?

src/libsyntax/ext/base.rs

Julian-Wollersberger · 2019-05-16T10:16:54Z

Implementing issue = "none" as a special case would be easy, but it becomes another corner case in (an unstable/internal part of) the language. I'd clearly expect the issue field to be consistent and only accept numbers.
On the other hand, I don't find it surprising that issue #0 means that there is no relevant issue.

This makes things worse, it's now much easier to forget the issue field.

With @petrochenkov 's concern in mind, making the field optional actually seems worse.

This change was suggested in #49985. @estebank and @GuillaumeGomez had opinions there.

varkor · 2019-06-03T00:00:06Z

Personally, I'm not too concerned about whether we use issue = "0" or issue = none or some other syntax for a missing issue (I have a slight preference to the latter, but the intent is mostly clear in either case.)

However, I do think using Option is better internally, because it separates the question of syntax from the semantics entirely (it's clear that None means "no issue", regardless of the exact syntax).

If there's not general consensus about the issue syntax, it'd be better to leave as-is, but change how it's represented. That way, we can move forward with this change.

@Julian-Wollersberger: sorry for not having figured out all the details before you made the changes!

bors · 2019-06-12T02:20:04Z

☔ The latest upstream changes (presumably #61741) made this pull request unmergeable. Please resolve the merge conflicts.

Julian-Wollersberger · 2019-06-20T11:28:18Z

I will then only change the internal representation and parse an issue = "0" to None, but leave the syntax as it is.

I'm sorry that this is taking me so long. I had exams last week and furthermore forgot my development laptop there. I hope I can continue next week.

joelpalmer · 2019-07-22T12:51:14Z

Ping from Triage. Hi @Julian-Wollersberger are there any updates for this PR? Thanks!

Dylan-DPC-zz · 2019-07-22T13:08:33Z

Closing this due to inactivity

@varkor

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

Julian-Wollersberger added 7 commits May 15, 2019 20:05

Began changing the type in StabilityLevel::Unstable from `issue: i3…

547e46a

…2` to `issue: Option<i32>` rust-lang#41260

Modified ui test cases and temporarily changed the type back to i32 t…

8a99c80

…o test them.

Types changed to issue: Option<i32> in a few places and I added a b…

6d68a6c

…asic ui test. It turned out that issue fields were already Option<i32> in most places. Added some temporary errors to find all the `#[unstable(... , issue = "0")]`

Made a warning for issues #0 and moved some code around.

6861c92

Removed most issue = "0" from ui tests.

43c8207

Commented out the warning.

68f1bf0

Blessed line number changes in ui tests.

eb2ebee

rust-highfive assigned varkor May 15, 2019

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

hellow554 reviewed May 16, 2019

View reviewed changes

src/librustc/session/mod.rs Show resolved Hide resolved

hellow554 reviewed May 16, 2019

View reviewed changes

src/libsyntax/attr/builtin.rs Show resolved Hide resolved

hellow554 reviewed May 16, 2019

View reviewed changes

src/libsyntax/ext/base.rs Show resolved Hide resolved

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2019

Dylan-DPC-zz closed this Jul 22, 2019

Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 22, 2019

rossmacarthur mentioned this pull request Nov 11, 2019

support issue = "none" in unstable attributes #66299

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `issue` field optional in `#[unstable]` to avoid `issue = "0"` #60860

Make `issue` field optional in `#[unstable]` to avoid `issue = "0"` #60860

Julian-Wollersberger commented May 15, 2019

rust-highfive commented May 15, 2019

rust-highfive commented May 15, 2019

petrochenkov commented May 15, 2019

varkor commented May 15, 2019 •

edited

Loading

hellow554 May 16, 2019

Julian-Wollersberger May 16, 2019 •

edited

Loading

hellow554 May 16, 2019

Julian-Wollersberger May 16, 2019

Julian-Wollersberger commented May 16, 2019

varkor commented Jun 3, 2019

bors commented Jun 12, 2019

Julian-Wollersberger commented Jun 20, 2019

joelpalmer commented Jul 22, 2019

Dylan-DPC-zz commented Jul 22, 2019

Make issue field optional in #[unstable] to avoid issue = "0" #60860

Make issue field optional in #[unstable] to avoid issue = "0" #60860

Conversation

Julian-Wollersberger commented May 15, 2019

rust-highfive commented May 15, 2019

rust-highfive commented May 15, 2019

petrochenkov commented May 15, 2019

varkor commented May 15, 2019 • edited Loading

hellow554 May 16, 2019

Choose a reason for hiding this comment

Julian-Wollersberger May 16, 2019 • edited Loading

Choose a reason for hiding this comment

hellow554 May 16, 2019

Choose a reason for hiding this comment

Julian-Wollersberger May 16, 2019

Choose a reason for hiding this comment

Julian-Wollersberger commented May 16, 2019

varkor commented Jun 3, 2019

bors commented Jun 12, 2019

Julian-Wollersberger commented Jun 20, 2019

joelpalmer commented Jul 22, 2019

Dylan-DPC-zz commented Jul 22, 2019

Make `issue` field optional in `#[unstable]` to avoid `issue = "0"` #60860

Make `issue` field optional in `#[unstable]` to avoid `issue = "0"` #60860

varkor commented May 15, 2019 •

edited

Loading

Julian-Wollersberger May 16, 2019 •

edited

Loading