Make pointer_structural_match normal and warn #110166

est31 · 2023-04-11T00:17:10Z

This PR does two things:

Changes the default level of pointer_structural_match from allow to warn
Turns pointer_structural_match into a normal lint instead of being a forward compatibility lint

The pointer_structural_match lint was added in #70743 to look for code that matches on a function pointer. As pointed out in #70861, and #54685, LLVM's behaviour makes function pointers behave in unexpected ways: function pointers might not be equal to "themselves" if function pointers for the same function were created in two different crates, and on the other hand, two different functions might end up with the same address if LLVM detects that they have the same body. See the linked issues for examples.

Thus, in #70743 an allow-by-default forward compatibility lint was added to address matching on pointers.

In #70861 (comment) preferences were expressed that there should be a lint for matching on function pointers, but it should not be disallowed completely. I'd interpret that as wanting a warn-by-default non forward compatibility lint. Therefore, there are two possible ways forward:

turn pointer_structural_match into a normal lint that's not a forward compatibility lint, and make it warn by default, OR
keep it as a forward compatibility lint, to eventually forbid it via a compiler error, but first make it warn by default, then deny by default.

I think one should at least do one of the two things, even if it is only going to be a warning lint in the end.

The lang team has given the green light for the first approach. There is still the possibility that it will be forbidden in the future, but this will require concrete deprecation plans which don't exist right now.

cc @eddyb @pnkfelix @oli-obk

est31 · 2023-04-11T05:20:29Z

cc also #74446 which contains some discussion of whether to allow fn pointers in match or not.

rustbot · 2023-04-11T05:20:54Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

oli-obk · 2023-04-14T10:25:57Z

r? lang

compiler/rustc_lint_defs/src/builtin.rs

tmandry · 2023-04-27T22:02:19Z

I tend to agree with the proposal to make it warn-by-default but not necessarily disallow it. That said, I think the lang team should discuss, so I nominated this PR for our next meeting.

nikomatsakis · 2023-05-09T22:02:03Z

We discussed this in our lang team meeting today. The consensus was:

We would like to warn about this.
We would prefer not to issue a FCW unless we have a specific plan to remove it, which at the moment doesn't exist. But we would be open to the idea of creating such a plan in the future, and we can go from warning to FCW.

This commit removes the pointer_structural_match forward compat lint from the forward compatibility warning mechanism, as there is no concrete plan for removal of matching on function pointers. It also turns the lint from allow-by-default to warn-by-default, as the behaviour might be surprising to users and they should specifically opt into the dangers. As the lint is recognized on older compilers, users can easily allow it after having made the descision on whether they want to keep it in their code or not.

est31 · 2023-05-09T23:32:02Z

I have changed the PR to now be warn by default and not be a forward compatibility lint.

@rustbot ready

tmandry · 2023-05-16T18:46:24Z

@rfcbot fcp merge

rfcbot · 2023-05-16T18:46:26Z

Team member @tmandry has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

nikomatsakis · 2023-05-16T18:46:43Z

@rfcbot fcp reviewed

rfcbot · 2023-05-16T18:47:40Z

🔔 This is now entering its final comment period, as per the review above. 🔔

compiler/rustc_lint_defs/src/builtin.rs

RalfJung · 2023-05-17T09:13:40Z

compiler/rustc_lint_defs/src/builtin.rs

-    /// compare as equal during runtime. This is because LLVM optimizations can deduplicate
-    /// functions if their bodies are the same, thus also making pointers to these functions point
-    /// to the same location. Additionally functions may get duplicated if they are instantiated
+    /// Use of function pointers and wide raw pointers in patterns works in many cases as


Suggested change

/// Use of function pointers and wide raw pointers in patterns works in many cases as

/// Use of function pointers and `dyn Trait` raw pointers in patterns works in many cases as

I assume this doesn't fire on slice raw pointers?

RalfJung · 2023-05-17T09:15:41Z

@oli-obk IIRC this was a future-compat lint because of our plans for match and value trees. With the current state of those plans, do we still need the future compat part? I assume no since #105750 landed. Function pointers and wide raw pointers do not have 'structural equality' but we allow them in match anyway because... backwards compatibility, I assume? In a perfect world we would only allow matching on things that have structural equality?

So I guess this means we have no plans to change that, and instead accept that we allow matching on some non-structural-eq types. We have a pretty clear semantics for that now (just use PartialEq) so that seems fine to me. But I wonder if the name and scope of the lint should be adjusted? The warning applies equally to == on function pointers and dyn Trait raw pointers. (Cc #69757)

est31 · 2023-05-17T09:27:37Z

With the current state of those plans, do we still need the future compat part? I assume no since #105750 landed.

There are a bunch of forward compat lints, including the one for floats, that might be removed if that's the direction you want to go in. Note that this PR is already removing the future compat part, but only for pointer_structural_match.

But I wonder if the name and scope of the lint should be adjusted? The warning applies equally to == on function pointers and dyn Trait raw pointers.

Hmm yeah good point, ideally we'd warn both for == and on matches, and then the name should probably be adjusted, too.

RalfJung · 2023-05-17T09:32:36Z

There are a bunch of forward compat lints, including the one for floats, that might be removed if that's the direction you want to go in. Note that this PR is already removing the future compat part, but only for pointer_structural_match.

Ah, I forgot about those. Either we plan to eventually reject these non-structural-eq-matches, then they should all remain future-compat lints, or we don't plan to reject such code, then we probably should remove the future-compat status for all of them.

Co-authored-by: Ralf Jung <post@ralfj.de>

rfcbot · 2023-05-26T18:48:36Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

est31 · 2023-05-27T10:48:06Z

So to summarize what @RalfJung suggested:

I should make this lint also warn on == comparisons
Probably a rename is needed... fn_pointer_comparisons, or, if dyn Trait is also affected, unstable_address_comparisons maybe?

RalfJung · 2023-09-17T09:54:04Z

@est31 what is the status of this? I recently dug into the entire pattern matching situation a bit more and I fully agree now that we don't want this to be a future-incompat lint any more. (And same for illegal_floating_point_literal_pattern.)

est31 · 2023-09-19T22:12:46Z

I need to find time to update this PR with the listed todos. If someone else wants to beat me to it, please be my guest.

Regarding illegal_floating_point_literal_pattern, do you think it should just lose the forward compat status but remain warn-by default? I don't have any preference in either direction, but there needs to be a new reason/story for why the lint is there, and why we don't warn for == float comparisons (warning for such would be extremely disruptive by default).

RalfJung · 2023-09-20T05:13:28Z

Yeah that's the thing, the entire justification and wording of the lint was in the anticipation of fwd compat changes. If those are not going to happen we should start from scratch, i.e., remove the lint and think about what we actually want to lint against and why, and have a new lint for that. That can happen in the same PR but it doesn't have to.

If we don't lint against == on floats I see no good reason to lint against match on floats.

Dylan-DPC · 2024-02-08T15:16:26Z

@est31 any updates on this?

RalfJung · 2024-02-08T18:44:42Z

I think this is superseded by #120423.

est31 · 2024-02-09T03:06:43Z

Yeah let's close it.

rustbot assigned oli-obk Apr 11, 2023

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 Apr 11, 2023

est31 force-pushed the pointer_structural_match branch from bf2ac4c to a562eaa Compare April 11, 2023 02:08

est31 force-pushed the pointer_structural_match branch from a562eaa to 679bf49 Compare April 11, 2023 05:20

oli-obk added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 14, 2023

rustbot assigned tmandry and unassigned oli-obk Apr 14, 2023

tmandry added the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 27, 2023

tmandry reviewed Apr 27, 2023

View reviewed changes

compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

est31 force-pushed the pointer_structural_match branch from e4c1f26 to 5bf1491 Compare April 28, 2023 00:50

est31 force-pushed the pointer_structural_match branch from 5bf1491 to 86b8a21 Compare May 9, 2023 22:40

est31 changed the title ~~Make pointer_structural_match lint warn by default~~ Make pointer_structural_match normal lint and warn by default May 9, 2023

est31 changed the title ~~Make pointer_structural_match normal lint and warn by default~~ Make pointer_structural_match normal and warn by default May 9, 2023

est31 changed the title ~~Make pointer_structural_match normal and warn by default~~ Make pointer_structural_match normal and warn May 9, 2023

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 16, 2023

rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label May 16, 2023

rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label May 16, 2023

RalfJung reviewed May 17, 2023

View reviewed changes

compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 17, 2023

View reviewed changes

est31 mentioned this pull request May 18, 2023

Uplift clippy::fn_null_check lint #111717

Merged

Also mention vtables

e8f39a4

Co-authored-by: Ralf Jung <post@ralfj.de>

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 26, 2023

rfcbot added the to-announce Announce this issue on triage meeting label May 26, 2023

est31 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 Jun 4, 2023

apiraino removed the to-announce Announce this issue on triage meeting label Jun 15, 2023

joshtriplett removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Aug 8, 2023

RalfJung mentioned this pull request Sep 15, 2023

Tracking issue for indirect_structural_match compatibility lint #62411

Closed

5 tasks

est31 closed this Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make pointer_structural_match normal and warn #110166

Make pointer_structural_match normal and warn #110166

est31 commented Apr 11, 2023 •

edited

Loading

est31 commented Apr 11, 2023

rustbot commented Apr 11, 2023

oli-obk commented Apr 14, 2023

tmandry commented Apr 27, 2023 •

edited

Loading

This comment has been minimized.

nikomatsakis commented May 9, 2023

est31 commented May 9, 2023

tmandry commented May 16, 2023

rfcbot commented May 16, 2023 •

edited by scottmcm

Loading

nikomatsakis commented May 16, 2023

rfcbot commented May 16, 2023

RalfJung May 17, 2023

RalfJung commented May 17, 2023 •

edited

Loading

est31 commented May 17, 2023

RalfJung commented May 17, 2023

rfcbot commented May 26, 2023

est31 commented May 27, 2023

RalfJung commented Sep 17, 2023

est31 commented Sep 19, 2023

RalfJung commented Sep 20, 2023

Dylan-DPC commented Feb 8, 2024

RalfJung commented Feb 8, 2024

est31 commented Feb 9, 2024

	/// Use of function pointers and wide raw pointers in patterns works in many cases as
	/// Use of function pointers and `dyn Trait` raw pointers in patterns works in many cases as

Make pointer_structural_match normal and warn #110166

Make pointer_structural_match normal and warn #110166

Conversation

est31 commented Apr 11, 2023 • edited Loading

est31 commented Apr 11, 2023

rustbot commented Apr 11, 2023

oli-obk commented Apr 14, 2023

tmandry commented Apr 27, 2023 • edited Loading

This comment has been minimized.

nikomatsakis commented May 9, 2023

est31 commented May 9, 2023

tmandry commented May 16, 2023

rfcbot commented May 16, 2023 • edited by scottmcm Loading

nikomatsakis commented May 16, 2023

rfcbot commented May 16, 2023

RalfJung May 17, 2023

Choose a reason for hiding this comment

RalfJung commented May 17, 2023 • edited Loading

est31 commented May 17, 2023

RalfJung commented May 17, 2023

rfcbot commented May 26, 2023

est31 commented May 27, 2023

RalfJung commented Sep 17, 2023

est31 commented Sep 19, 2023

RalfJung commented Sep 20, 2023

Dylan-DPC commented Feb 8, 2024

RalfJung commented Feb 8, 2024

est31 commented Feb 9, 2024

est31 commented Apr 11, 2023 •

edited

Loading

tmandry commented Apr 27, 2023 •

edited

Loading

rfcbot commented May 16, 2023 •

edited by scottmcm

Loading

RalfJung commented May 17, 2023 •

edited

Loading