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

Allow multiple allow_internal_unstable attributes #77183

Merged
merged 1 commit into from
Sep 25, 2020
Merged

Allow multiple allow_internal_unstable attributes #77183

merged 1 commit into from
Sep 25, 2020

Conversation

bugadani
Copy link
Contributor

Fixes #77088

@rust-highfive
Copy link
Collaborator

r? @varkor

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2020
varkor
varkor reviewed Sep 25, 2020
View reviewed changes
Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

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

r=me after adjusting the error text.

compiler/rustc_attr/src/builtin.rs Outdated Show resolved Hide resolved
@bugadani @varkor
Allow multiple allow_internal_unstable attributes
54c9c94 
Co-authored-by: varkor <github@varkor.com>
@bugadani
Copy link
Contributor Author

@varkor it should be good now, I've also squashed the changes

@varkor
Copy link
Member

varkor commented Sep 25, 2020

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 25, 2020

📌 Commit 54c9c94 has been approved by varkor

@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 Sep 25, 2020
@jonas-schievink jonas-schievink mentioned this pull request Sep 25, 2020
Closed
This was referenced Sep 25, 2020
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 25, 2020
@bors
Auto merge of rust-lang#77198 - jonas-schievink:rollup-i59i41h, r=jon…
c6e4db6 
…as-schievink

Rollup of 15 pull requests

Successful merges:

 - rust-lang#76932 (Relax promises about condition variable.)
 - rust-lang#76973 (Unstably allow assume intrinsic in const contexts)
 - rust-lang#77005 (BtreeMap: refactoring around edges)
 - rust-lang#77066 (Fix dest prop miscompilation around references)
 - rust-lang#77073 (dead_code: look at trait impls even if they don't contain items)
 - rust-lang#77086 (Include libunwind in the rust-src component.)
 - rust-lang#77097 (Make [].as_[mut_]ptr_range() (unstably) const.)
 - rust-lang#77106 (clarify that `changelog-seen = 1` goes to the beginning of config.toml)
 - rust-lang#77120 (Add `--keep-stage-std` to `x.py` for keeping only standard library artifacts)
 - rust-lang#77126 (Invalidate local LLVM cache less often)
 - rust-lang#77146 (Install std for non-host targets)
 - rust-lang#77155 (remove enum name from ImplSource variants)
 - rust-lang#77176 (Removing erroneous semicolon in transmute documentation)
 - rust-lang#77183 (Allow multiple allow_internal_unstable attributes)
 - rust-lang#77189 (Remove extra space from vec drawing)

Failed merges:

r? `@ghost`
@bors bors merged commit 12b8d89 into rust-lang:master Sep 25, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 25, 2020
@bugadani bugadani deleted the issue-77088 branch September 25, 2020 21:46
@RalfJung
Copy link
Member

Cc @rust-lang/wg-const-eval

oli-obk
oli-obk reviewed Sep 26, 2020
View reviewed changes
compiler/rustc_attr/src/builtin.rs
.span_err(attr.span, "allow_internal_unstable expects list of feature names");
None
})?;
let attrs = sess.filter_by_name(attrs, sym::allow_internal_unstable);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how this PR fixes anything. All you've done is replace a ? operator with a filter map. But find_by_name will still only return a single attribute.

Please revert the PR. I'll reopen the issue, then we can discuss an actual fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not find_by_name, but filter_by_name which returns an iterator with all matching attributes, instead of the first one.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤦 I apologize. I should not review half asleep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fine. I'll check locally if the test case I added actually fails without

@oli-obk oli-obk mentioned this pull request Sep 26, 2020
Closed
@bugadani
Copy link
Contributor Author

@oli-obk

Without the changes, the added test case fails with the following (diff of stderr):

-       error: aborting due to 5 previous errors
+       error[E0658]: use of unstable library feature 'struct2_field'
+         --> $DIR/internal-unstable.rs:31:35
+          |
+       LL |     |x: internal_unstable::Bar| { access_field_allow2!(x) }; // regression test for #77088
+          |                                   ^^^^^^^^^^^^^^^^^^^^^^^
+          |
+          = help: add `#![feature(struct2_field)]` to the crate attributes to enable
+          = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+       error: aborting due to 6 previous errors

Yesterday, I had some trouble with this, the changes didn't seem to apply and the test case failed regardless. It seemed for a long time that even getting the list of attributes returned only 1. After wasting an hour and a half, I realized that I added struct_field2 to the macro, instead of struct2_field. 🤷‍♀️ I guess this piece of code is cursed.

Thanks for raising concerns, it's always good to double check.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 26, 2020

Thanks for double checking!

@RalfJung
Copy link
Member

Does this change fix both macros and stable const fn using #[allow_internal_unstable]?

@bugadani
Copy link
Contributor Author

Does this change fix both macros and stable const fn using #[allow_internal_unstable]?

I'm not sure about const fn because I don't know which features would apply there and I had trouble with allowing the unchecked_unreachable hint. As far as I can see, the feature gates are checked through this piece of code, but I'm not sure if const fn actually accepts the features enabled this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@varkor varkor varkor left review comments

Assignees

@varkor varkor

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.

allow_internal_unstable attr only allowed once per function
7 participants
@bugadani @rust-highfive @varkor @bors @RalfJung @oli-obk @rustbot