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

Do not call fn_sig on non-functions. #105201

Merged
merged 1 commit into from
Dec 4, 2022
Merged

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Dec 3, 2022

Fixes #105040
Fixes #89271

@rustbot
Copy link
Collaborator

rustbot commented Dec 3, 2022

r? @davidtwco

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 3, 2022
@matthiaskrgr
Copy link
Member

Does this also fix #89271 ?

@cjgillot
Copy link
Contributor Author

cjgillot commented Dec 3, 2022

Yes.

Copy link
Member

@compiler-errors compiler-errors 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 comment addressed or not

// Do not call `fn_sig` on non-functions.
if !matches!(
self.tcx.def_kind(def_id),
DefKind::Fn | DefKind::AssocFn | DefKind::Variant | DefKind::Ctor(..)
Copy link
Member

Choose a reason for hiding this comment

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

Hm, what about closures? Side-note, there's DefKind::is_fn_like, but it's slightly different that this case... Maybe worth checking use-cases and syncing them up.

Copy link
Member

Choose a reason for hiding this comment

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

Wait, closures make no sense -- they have no where clauses, lol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling fn_sig on closures and generators ICEs too.

@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 3, 2022

📌 Commit e973240 has been approved by compiler-errors

It is now in the queue for this repository.

@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 3, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2022
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104199 (Keep track of the start of the argument block of a closure)
 - rust-lang#105050 (Remove useless borrows and derefs)
 - rust-lang#105153 (Create a hacky fail-fast mode that stops tests at the first failure)
 - rust-lang#105164 (Restore `use` suggestion for `dyn` method call requiring `Sized`)
 - rust-lang#105193 (Disable coverage instrumentation for naked functions)
 - rust-lang#105200 (Remove useless filter in unused extern crate check.)
 - rust-lang#105201 (Do not call fn_sig on non-functions.)
 - rust-lang#105208 (Add AmbiguityError for inconsistent resolution for an import)
 - rust-lang#105214 (update Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f91fa51 into rust-lang:master Dec 4, 2022
@rustbot rustbot added this to the 1.67.0 milestone Dec 4, 2022
@cjgillot cjgillot deleted the issue-105040 branch December 4, 2022 08:17
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 6, 2022
…fn-ptr-arg, r=cjgillot

Point at args in associated const fn pointers

Tiny follow-up to rust-lang#105201, not so sure it's worth it but 🤷

The UI test example is a bit more compelling when it's `GlUniformScalar::FACTORY`

r? `@cjgillot`
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 26, 2022
…r=cjgillot

Don't perform invalid checks in `codegen_attrs`

The attributes `#[track_caller]` and `#[cmse_nonsecure_entry]` are only valid on functions. When validating one of these attributes, codegen_attrs previously called `fn_sig`, [which can only be used on functions](rust-lang#105201), on the item the attribute was attached to, assuming that the item was a function without checking. This led to [ICEs in situations where the attribute was incorrectly used on non-functions](rust-lang#105594).

With this change, we skip calling `fn_sig` if the item the attribute is attached to must be a function but isn't, because `check_attr` will reject such cases without codegen_attrs's intervention.

As a side note, some of the attributes in codegen_attrs are only valid on functions, but that property isn't actually checked. I'm planning to fix that in a follow up PR since it's a behavior change that will need to be validated rather than an obvious bugfix. Thankfully, all the attributes like that I've found so far are unstable.

Fixes rust-lang#105594.

r? `@cjgillot`
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Dec 28, 2022
Don't perform invalid checks in `codegen_attrs`

The attributes `#[track_caller]` and `#[cmse_nonsecure_entry]` are only valid on functions. When validating one of these attributes, codegen_attrs previously called `fn_sig`, [which can only be used on functions](rust-lang/rust#105201), on the item the attribute was attached to, assuming that the item was a function without checking. This led to [ICEs in situations where the attribute was incorrectly used on non-functions](rust-lang/rust#105594).

With this change, we skip calling `fn_sig` if the item the attribute is attached to must be a function but isn't, because `check_attr` will reject such cases without codegen_attrs's intervention.

As a side note, some of the attributes in codegen_attrs are only valid on functions, but that property isn't actually checked. I'm planning to fix that in a follow up PR since it's a behavior change that will need to be validated rather than an obvious bugfix. Thankfully, all the attributes like that I've found so far are unstable.

Fixes #105594.

r? `@cjgillot`
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104199 (Keep track of the start of the argument block of a closure)
 - rust-lang#105050 (Remove useless borrows and derefs)
 - rust-lang#105153 (Create a hacky fail-fast mode that stops tests at the first failure)
 - rust-lang#105164 (Restore `use` suggestion for `dyn` method call requiring `Sized`)
 - rust-lang#105193 (Disable coverage instrumentation for naked functions)
 - rust-lang#105200 (Remove useless filter in unused extern crate check.)
 - rust-lang#105201 (Do not call fn_sig on non-functions.)
 - rust-lang#105208 (Add AmbiguityError for inconsistent resolution for an import)
 - rust-lang#105214 (update Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
6 participants