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

Strict provenance lints #95599

Merged
merged 2 commits into from
Apr 9, 2022
Merged

Conversation

niluxv
Copy link
Contributor

@niluxv niluxv commented Apr 2, 2022

See #95488.
This PR introduces two unstable (allow by default) lints to which lint on int2ptr and ptr2int casts, as the former is not possible in the strict provenance model and the latter can be written nicer using the .addr() API.
Based on an initial version of the lint by @Gankra in #95199.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 2, 2022
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(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 Apr 2, 2022
@rust-log-analyzer

This comment has been minimized.

compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/cast.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@niluxv
Copy link
Contributor Author

niluxv commented Apr 3, 2022

Will address the comments later today.

@niluxv niluxv marked this pull request as ready for review April 3, 2022 14:16
@niluxv
Copy link
Contributor Author

niluxv commented Apr 3, 2022

I can do a follow up PR later to enable these lints in the std library crates and compiler crates. Main question right now is whether to use a separate feature gate or the strict_provenance feature gate for these lints.

@niluxv niluxv changed the title WIP: Strict provenance lint Strict provenance lints Apr 3, 2022
@Gankra
Copy link
Contributor

Gankra commented Apr 3, 2022

credit where it's due: @eddyb actually wrote most of the code here, I mostly just wrote the docs.

@niluxv
Copy link
Contributor Author

niluxv commented Apr 3, 2022

Seems like things are moving in #95588 so I will adjust docs and suggestions hopefully tomorrow.

@niluxv
Copy link
Contributor Author

niluxv commented Apr 4, 2022

Done. Also fixed suggestions for ptr2int non-usize casts. Now really ready for review I guess. I can rebase it into a nice history at the end.

@niluxv
Copy link
Contributor Author

niluxv commented Apr 4, 2022

@rustbot label A-strict-provenance

@rustbot rustbot added the A-strict-provenance Area: Strict provenance for raw pointers label Apr 4, 2022
@michaelwoerister
Copy link
Member

I'll try to take a look at this shortly.

Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @niluxv! I left some comments below. But under the assumption that @Gankra has taken a look at this too, this seems basically good to merge, I'd say.

Main question right now is whether to use a separate feature gate or the strict_provenance feature gate for these lints.

I think it's fine to use the same feature gate -- but if there turns out to be a problem with that, we can split it out later.

compiler/rustc_lint_defs/src/builtin.rs Show resolved Hide resolved
compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
err.span_suggestion(
self.span,
msg,
format!("({}).addr(){}", snippet, scalar_cast),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This suggestion can contain unnecessary parenthesis but I have no idea how to reliably check whether snippet requires enclosing parenthesis, and also don't know whether fixing that would be worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because we have access to the hir::Node::Expr we can check its op_precedence. We do it in some diagnostics already. It's also usually better to use a multipart_suggestion instead of relying on getting the code's snippet when possible. We used not to do that because the output is more verbose, but it is so much easier to read that I think it is worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the hints. Opened #96112.

--> $DIR/lint-strict-provenance-lossy-casts.rs:9:22
|
LL | let addr_32bit = &x as *const u8 as u32;
| ^^^^^^^^^^^^^^^^^^^^^^ help: use `.addr()` to obtain the address of a pointer: `(&x as *const u8).addr() as u32`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As seen in this example the suggestion lines tend to get really long. Feels suboptimal, but I don't know how to change the way the suggestion is shown.

@michaelwoerister
Copy link
Member

I think this can be merged in the current state (with the commit history squashed/cleaned up).

The remaining issues seem like minor diagnostics polish, which can be done in a follow up PR.

Gankra and others added 2 commits April 8, 2022 17:40
… test it

* split `fuzzy_provenance_casts` into a ptr2int and a int2ptr lint
* feature gate both lints
* update documentation to be more realistic short term
* add tests for these lints
@niluxv
Copy link
Contributor Author

niluxv commented Apr 8, 2022

Rebased onto master and squashed all my commits together (so it is 2 commits now).

@michaelwoerister
Copy link
Member

Thanks, @niluxv!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 8, 2022

📌 Commit 98a4834 has been approved by michaelwoerister

@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 Apr 8, 2022
err.help(msg);
}
err.help(
"if you can't comply with strict provenance and need to expose the pointer\
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"if you can't comply with strict provenance and need to expose the pointer\
"if you can't comply with strict provenance and need to expose the pointer \

Currently it is printed without a space, I doubt that is intentional. (Test output also needs to be adjusted then.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm too late to modify this PR. Should I open a new one?

Copy link
Member

Choose a reason for hiding this comment

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

Yes that'd be good. :)

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 8, 2022
…chaelwoerister

Strict provenance lints

See rust-lang#95488.
This PR introduces two unstable (allow by default) lints to which lint on int2ptr and ptr2int casts, as the former is not possible in the strict provenance model and the latter can be written nicer using the `.addr()` API.
Based on an initial version of the lint by `@Gankra` in rust-lang#95199.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 9, 2022
…chaelwoerister

Strict provenance lints

See rust-lang#95488.
This PR introduces two unstable (allow by default) lints to which lint on int2ptr and ptr2int casts, as the former is not possible in the strict provenance model and the latter can be written nicer using the `.addr()` API.
Based on an initial version of the lint by ``@Gankra`` in rust-lang#95199.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2022
Rollup of 8 pull requests

Successful merges:

 - rust-lang#90066 (Add new ThinBox type for 1 stack pointer wide heap allocated trait objects)
 - rust-lang#95374 (assert_uninit_valid: ensure we detect at least arrays of uninhabited types)
 - rust-lang#95599 (Strict provenance lints)
 - rust-lang#95751 (Don't report numeric inference ambiguity when we have previous errors)
 - rust-lang#95764 ([macro_metavar_expr] Add tests to ensure the feature requirement)
 - rust-lang#95787 (reword panic vs result section to remove recoverable vs unrecoverable framing)
 - rust-lang#95797 (Remove explicit delimiter token trees from `Delimited`.)
 - rust-lang#95804 (rustdoc: Fix empty doc comment with backline ICE)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 525438b into rust-lang:master Apr 9, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 9, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 9, 2022
…, r=Dylan-DPC

Fix missing space in lossy provenance cast lint

See rust-lang#95599 (comment)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 10, 2022
…, r=Dylan-DPC

Fix missing space in lossy provenance cast lint

See rust-lang#95599 (comment)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 10, 2022
…, r=Dylan-DPC

Fix missing space in lossy provenance cast lint

See rust-lang#95599 (comment)
@Gankra
Copy link
Contributor

Gankra commented Apr 23, 2022

thanks so much @niluxv!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-strict-provenance Area: Strict provenance for raw pointers 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
Development

Successfully merging this pull request may close these issues.

10 participants