Skip to content

Slew of builtin-attribute gating tests #43168

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

Merged
merged 1 commit into from
Jul 19, 2017

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Jul 11, 2017

Slew of builtin-attribute "gating" tests for issue #43106.

Some stray observations:

  • I don't know if its a good thing that so many attributes allow inputs which are silently discarded. (I made heavy use of that in writing my tests, but that was more out of curiosity than necessity.)
  • The difference between crate-level and non-crate-level behavior is quite significant in some cases. Definitely worth making sure one has tests for both cases. (Not as clear whether it was worthwhile trying the various other AST forms like fn f() vs struct S;)
  • #[no_builtins] and #[no_mangle] occur twice on the BUILTIN_ATTRIBUTES list. Thats almost certainly a bug. (Filed as #![no_builtins] attribute occurs twice on BUILTIN_ATTRIBUTES list #43148)
  • We are maximally liberal in what we allow for #[test] and #[bench] when one compiles without --test.
  • We allow #[no_mangle] on arbitrary AST nodes, but only warn about potential misuse on fn
  • We allow #[cold], #[must_use], #[windows_subsystem], and #[no_builtins] on arbitrary AST nodes. I don't know off-hand what the semantics are for e.g. a #[cold] type T = ...;
  • We allow crate-level #![inline]. That's probably a bug since its otherwise restricted to fn items

@rust-highfive
Copy link
Contributor

r? @aturon

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

@pnkfelix
Copy link
Member Author

Oh, and just in case there's any debate about what the "point" is of these tests, this comment from issue-43106-gating-of-builtin-attrs.rs is probably worth quoting explicitly:

This test enumerates as many compiler-builtin ungated attributes as possible (that is, all the mutually compatible ones), and checks that we get "expected" (*) warnings for each in the various weird places that users might put them in the syntax.

(*): The word "expected" is in quotes above because the cases where warnings are and are not emitted might not match a user's intuition nor the rustc developers' intent. I am really just trying to capture today's behavior in a test, not so that it become enshrined as the absolute behavior going forward, but rather so that we do not change the behavior in the future without even being aware of the change when it happens.

@pnkfelix pnkfelix changed the title Slew of feature gating tests Slew of builtin-attribute gating tests Jul 11, 2017
@arielb1 arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 11, 2017
@Mark-Simulacrum
Copy link
Member

@aturon Looks like this is waiting on a review from you. Also pinged on IRC.

@aturon
Copy link
Member

aturon commented Jul 17, 2017

@bors: r+

@bors
Copy link
Collaborator

bors commented Jul 17, 2017

📌 Commit 39b8aaf has been approved by aturon

@bors
Copy link
Collaborator

bors commented Jul 18, 2017

⌛ Testing commit 39b8aaf with merge 16068c512dcfc0dea5290f625b93b1da49702b61...

@bors
Copy link
Collaborator

bors commented Jul 18, 2017

💔 Test failed - status-travis

@est31
Copy link
Member

est31 commented Jul 18, 2017

@bors retry

@bors
Copy link
Collaborator

bors commented Jul 18, 2017

⌛ Testing commit 39b8aaf with merge cbbf509e94e1b24a3c4026963544918a85f8e03c...

@bors
Copy link
Collaborator

bors commented Jul 18, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing cbbf509e94e1b24a3c4026963544918a85f8e03c to master...

@bors
Copy link
Collaborator

bors commented Jul 18, 2017

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

@Mark-Simulacrum
Copy link
Member

@bors retry

@arielb1 arielb1 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 Jul 18, 2017
@bors
Copy link
Collaborator

bors commented Jul 19, 2017

⌛ Testing commit 39b8aaf with merge 88e2c39...

bors added a commit that referenced this pull request Jul 19, 2017
Slew of builtin-attribute gating tests

Slew of builtin-attribute "gating" tests for issue #43106.

Some stray observations:

 * I don't know if its a good thing that so many attributes allow inputs which are silently discarded. (I  made heavy use of that in writing my tests, but that was more out of curiosity than necessity.)
 * The difference between crate-level and non-crate-level behavior is quite significant in some cases. Definitely worth making sure one has tests for both cases. (Not as clear whether it was worthwhile trying the various other AST forms like `fn f()` vs `struct S;`)
 * `#[no_builtins]` and `#[no_mangle]` occur twice on the `BUILTIN_ATTRIBUTES` list. Thats almost certainly a bug. (Filed as #43148)
 * We are maximally liberal in what we allow for `#[test]` and `#[bench]` when one compiles without `--test`.
 * We allow `#[no_mangle]` on arbitrary AST nodes, but only warn about potential misuse on `fn`
 * We allow `#[cold]`, `#[must_use]`, `#[windows_subsystem]`, and `#[no_builtins]` on arbitrary AST nodes. I don't know off-hand what the semantics are for e.g. a `#[cold] type T = ...;`
 * We allow crate-level `#![inline]`. That's probably a bug since its otherwise restricted to `fn` items
@bors
Copy link
Collaborator

bors commented Jul 19, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 88e2c39 to master...

@bors bors merged commit 39b8aaf into rust-lang:master Jul 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants