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

use visitor for #[structural_match] check #62339

Conversation

@pnkfelix
Copy link
Member

commented Jul 3, 2019

This changes the code so that we recur down the structure of a type of a const (rather than just inspecting at a shallow one or two levels) when we are looking to see if it has an ADT that did not derive PartialEq and Eq.

Fix #61188

Fix #62307

Cc #62336

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 3, 2019

r? @varkor

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

@pnkfelix

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

The way this is written might be bad for performance, since it is doing a visit of the type structure on every call to const_to_pat, and that routine calls itself recursively.

Thinking further on this, we might be able to avoid that cost by factoring the recursive traversal into a subroutine, and then doing the type traversal once, at the start of const_to_pat, before starting the recursive traversal. (But I would want to be careful about not overlooking cases where the problematic ADT occurs more deeply in the input.)

@pnkfelix pnkfelix changed the title use visitor for #[structural_match] check [WIP] use visitor for #[structural_match] check Jul 3, 2019

@pnkfelix

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

(I've left the WIP marker because I want to add tests that double-check the refactoring in commit 9db458f didn't break things.)

@Centril
Copy link
Member

left a comment

Just some nits...

@@ -1058,6 +1067,52 @@ impl<'a, 'tcx> PatternContext<'a, 'tcx> {
}
}

fn search_for_adt_without_structural_match<'tcx>(tcx: TyCtxt<'tcx>,

This comment has been minimized.

Copy link
@Centril

Centril Jul 3, 2019

Member

Indentation is weird here, should be:

fn search_for_adt_without_structural_match<'tcx>(
    tcx: TyCtxt<'tcx>,
    ty: Ty<'tcx>,
) -> Option<&'tcx AdtDef> {
    ...
Show resolved Hide resolved src/librustc_mir/hair/pattern/mod.rs Outdated
Show resolved Hide resolved src/librustc_mir/hair/pattern/mod.rs Outdated
Show resolved Hide resolved src/librustc_mir/hair/pattern/mod.rs Outdated
Show resolved Hide resolved src/test/ui/rfc1445/issue-61118-match-slice-forbidden-without-eq.rs Outdated
Show resolved Hide resolved src/test/ui/rfc1445/issue-61118-match-slice-forbidden-without-eq.rs Outdated
Show resolved Hide resolved src/test/ui/rfc1445/issue-62307-match-ref-ref-forbidden-without-eq.rs Outdated
Show resolved Hide resolved src/test/ui/rfc1445/issue-62307-match-ref-ref-forbidden-without-eq.rs Outdated
Show resolved Hide resolved src/test/ui/rfc1445/match-nonempty-array-forbidden-without-eq.rs Outdated
@matthewjasper

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

Does this handle matching on const A: &[Option<NoDerive>] = ...?

@pnkfelix

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

@matthewjasper no, it mishandles the case of const A: &[Option<NoDerive>] (as in, it fails to reject a pattern using that const).

The code also mishandles some other cases that I just discovered, some of which are regressions over the current master. So I need to fix that, and add corresponding regression tests.

  • I wanted to double check the text of rust-lang/rfcs#1445 to make sure I understand the intended scope of that change
  • its possible the RFC intended for us to use a shallow check of the attribute; i.e., that we would accept the case of a struct wrapping another struct, where the wrapper derives PartialEq and Eq but the wrapped type does not derive them and instead uses its own implementation.
  • But I don't think that could have been the true intention, since allowing such side-stepping of the attribute via a wrapper struct seems like it would undermine the objectives of the RFC itself.
  • (I suspect the reality is the RFC was written assuming a sort of recursive semantics, where converting the wrapper to a pattern would involve converting the wrapped struct to a pattern, which in turn would cause the wrapped struct to be checked. But this is not what currently happens, at least for &T, due to the way we sometimes dispatch to a PartialEq::eq call rather than expanding.)

I'll post a follow-up comment outlining the behavior on a set of cases after I finishing going through the analysis.

@pnkfelix

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

Okay, that change 9db458f (to move the visit up above the recursive loop) broke (as in regressed) cases like these:

struct NoDerive(i32);

impl PartialEq for NoDerive {
    fn eq(&self, _: &Self) -> bool { false }
}

impl Eq for NoDerive { }

#[derive(PartialEq, Eq)]
struct WrapInline(NoDerive);

const WRAP_DIRECT_INLINE: WrapInline = WrapInline(NoDerive(0));

#[derive(PartialEq, Eq)]
struct WrapParam<T>(T);

const WRAP_DIRECT_PARAM: WrapParam<NoDerive> = WrapParam(NoDerive(0));

Independently of that change, regardless of where the visit is done, I/we also fail to correctly handle:

const WRAP_INDIRECT_INLINE: & &WrapInline = & &WrapInline(NoDerive(0));

const WRAP_INDIRECT_PARAM: & &WrapParam<NoDerive> = & &WrapParam(NoDerive(0));

@pnkfelix pnkfelix force-pushed the pnkfelix:issue-61188-use-visitor-for-structural-match-check branch from fb518f1 to 1e55df7 Jul 4, 2019

@pnkfelix

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

Okay so now I've removed commit 9db458f to avoid regressing cases that we were handling correctly before, and I've added 1e55df7 because I've decided that we do need to visit the substs of an ADT (and just treat PhantomData<T> as a special case).

With that change in place, we handle @matthewjasper 's example correctly, as well as most of the other ones I identified.

The one case that this version continues to mishandle is this (play):

struct NoDerive;

impl PartialEq for NoDerive {
    fn eq(&self, _: &Self) -> bool { false }
}

impl Eq for NoDerive { }

#[derive(PartialEq, Eq)]
struct WrapInline(NoDerive);

const WRAP_INLINE: & &WrapInline = & &WrapInline(NoDerive);

fn main() {
    match WRAP_INLINE {
        WRAP_INLINE => { println!("WRAP_INLINE matched WRAP_INLINE"); }
        _ => { println!("WRAP_INLINE did not match WRAP_INLINE"); }
    }
}

I think handling this case correctly (and also efficiently) will require deeper changes, though. E.g. a new bit in rustc::ty::TypeFlags.

  • To be honest we might be better off trying to address finding our long-term solution to the structural-match problem rather than trying to plug all the leaks in #[structural_match]...
  • But under the assumption that gathering consensus on a language problem like that will always take more time than you expect, I'll see about changing rustc::ty::TypeFlags in the short term.
  • Update: I found a simpler way to deal with it: just traverse the fields and use a seen map for the &AdtDef's we've seen. (I wonder how many other passes are reimplementing this graph traversal themselves, either with seen: HashSet<&AdtDef> or HashSet<Ty>)
@pnkfelix

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

Okay, so now I've added a more complete traversal that addresses the problem posted above.

But doing that then led me to ask some questions, like:

  • "Now that I am traversing the fields, should I stop traversing the substs of an ADT?"
    • (probably yes, especially given the next bullet...)
  • "How should unsafe pointers be handled?"
    • (probably should not recur on the T in *const T)

The fact that I'm asking these questions leads me to think three things:

  • I need to add more tests
  • The implementation here needs more work
  • This probably needs to be initially deployed with a warning cycle. (I don't think that will be too hard to do, even.)
@Centril

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

r? @nikomatsakis since they wrote the original RFC.

@rust-highfive rust-highfive assigned nikomatsakis and unassigned varkor Jul 5, 2019

@pnkfelix

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

Turning this from an error to a warning exposes more issues; e.g. the point where we are doing this check in the compiler's control flow, namely lowering from HIR to HAIR, is a spot where for some reason we may attempt to lower the same input multiple times, and thus the warning gets issued redundantly.

(The very fact that we are re-running lower on the same input seems like a worrisome thing, just in terms of compiler efficiency...)

So, do I focus on making this a LintPass rather than doing it in lowering? Or do I try to figure out why lowering is being run multiple times on the same input?

  • Update: the answer about the latter seems to be that we call lower_pattern from both hair::pattern::Pattern::from_hir and from hair::pattern::check_match. I guess that should be at most a 2x cost, and most patterns tend to be small. (But it also strikes me as unfortunate redundant work.)
  • In any case, given the answer above, it seems like I can readily resolve my issue by having the PatternContext know whether it should issue lints or not (and then the different contexts that create it can adjust it accordingly.)
@pnkfelix

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

Okay I'm finally narrowing in on a solution that I'm happy with, I hope.

I am going to try to deploy this as a future-compat lint; I've filed #62411 as a tracking issue for that.

pnkfelix added some commits Jul 8, 2019

Rewrite with future-compat lint for indirect pattern omitting
`#[structural_match]`.

Outline of changes:

 * Recur as deeply as necessary when searching for `#[structural_match]`.

 * `#[structural_match]`: handle case of `const A: & &Wrap(NoDerive)`
   by including the fields of an ADT during traversal of input
   type. (We continue to not traverse the substs of an ADT, though, so
   that we continue to handle `PhantomData<NoDerive>` and `*NoDerive`
   properly.)

 * Refactored code to use `match` instead of `if let`. This ends up
   *with less* right-ward drift by moving the handling of the main
   *`ty::Adt` case *outside* the match.

 * Using lint (rather than hard error) mmeans we need to check that
   type is `PartialEq` to avoid ICE'ing the compiler in scneario where
   MIR codegen dispatches to `PartialEq::eq`. Added said check, and
   fatal error in that case.

@pnkfelix pnkfelix force-pushed the pnkfelix:issue-61188-use-visitor-for-structural-match-check branch from 732b54c to c7a6a5a Jul 8, 2019

Regression tests and updates to existing tests.
The regression tests explore:
  (direct | indirect | doubly-indirect | unsafe) x (embedded | param):

where:
  embedded: `struct Wrapper(... NoDerive ...);`
  param:    `struct Wrapper<X>(... X ...);`

  direct:          `const A:     Wrapper<...> = Wrapper(NoDerive);`
  indirect:        `const A: & & Wrapper<...> = Wrapper(NoDerive)`
  doubly-indirect: `const A: & & Wrapper<...> = & & Wrapper(& & NoDerive)`
  unsafe:          `const A: UnsafeWrap<...>  = UnsafeWrap(std::ptr::null())`

@pnkfelix pnkfelix force-pushed the pnkfelix:issue-61188-use-visitor-for-structural-match-check branch from c7a6a5a to 02714b8 Jul 8, 2019

@pnkfelix pnkfelix changed the title [WIP] use visitor for #[structural_match] check use visitor for #[structural_match] check Jul 8, 2019

@pnkfelix

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

(last commit 02af3ca was result of my realization that while I had added special case code to preserve existing behavor for empty-arrays (#62336), I hadn't put in a corresponding unit test. That's fixed now.)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

My 2 cents:

  • r=me on the PR itself.
  • it seems like a good idea to do a crater run (with this lint level set to deny) to get an idea of the impact.
  • I do think we ought to resolve this question around constants. This feels like something where, if we had a functional working group around const generics, they could ultimately resolve it.
@pnkfelix

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

hmm, looking at #36891, I now wonder: should we use a different name than non_structural_match for the future-compat lint here? I had considered e.g. indirect_non_structural_match...

Having said that, I nonetheless plan to let this PR land as is. We can bikeshed the names while the PR is on nightly (right?)

Update: ha ha, I'm losing my marbles; the PR has already named the lint indirect_structural_match. Its just the tracking issue for the lint that has the wrong name.

@pnkfelix

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

@bors r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

📌 Commit b0b64dd has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

⌛️ Testing commit b0b64dd with merge c6a9e76...

bors added a commit that referenced this pull request Jul 10, 2019

Auto merge of #62339 - pnkfelix:issue-61188-use-visitor-for-structura…
…l-match-check, r=nikomatsakis

use visitor for #[structural_match] check

This changes the code so that we recur down the structure of a type of a const (rather than just inspecting at a shallow one or two levels) when we are looking to see if it has an ADT that did not derive `PartialEq` and `Eq`.

Fix #61188

Fix #62307

Cc #62336
@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

☀️ Test successful - checks-azure, checks-travis, status-appveyor
Approved by: nikomatsakis
Pushing c6a9e76 to master...

@bors bors added the merged-by-bors label Jul 10, 2019

@bors bors merged commit b0b64dd into rust-lang:master Jul 10, 2019

4 checks passed

homu Test successful
Details
pr Build #20190709.37 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details

@jethrogb jethrogb referenced this pull request Jul 12, 2019

Open

Restrict use of constants in patterns (RFC 1445) #31434

3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.