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

ast: the id of a ref pattern conflated; has type `T` in some contexts and `&T` in others #18207

Closed
pnkfelix opened this Issue Oct 21, 2014 · 7 comments

Comments

Projects
None yet
5 participants
@pnkfelix
Copy link
Member

pnkfelix commented Oct 21, 2014

While working on my follow-up to #17439 I rediscovered an old problem in our ast representation:

We usually act like you can unambiguously map a node_id to a single type for that node id. However, if you want to know the type of a pattern like the ref x in:

let t = Some(box 3i);
match t {
  Some(ref x) => ...,
  None => ...,
}

then you have a problem: are you trying to find the type of the thing that is being borrowed by the match (i.e. int in this case)? Or are you trying to find the type of x itself (&int in this case)?

The problem is that there is just one node_id for the entire ref <id> pattern, even though there are two distinct things that you are potentially trying to match.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Oct 27, 2015

@pnkfelix is this issue still valid?

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Oct 27, 2015

@steveklabnik IMO it still is an issue. Its something internal to the compiler, but I wouldn't be surprised if it is a source of bugs (either old or new.)

cc @rust-lang/compiler

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Oct 27, 2015

@pnkfelix

This is semi-going-away with MIR, so not particularly important.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Oct 27, 2015

@arielb1 should we have tags for that sort of "fixed-by-MIR" thing?

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Oct 29, 2015

Hmm I will need to review the MIR RFC ... while I understand that some analyses will be applicable to the MIR (and indeed, some of those will solely apply to MIR then, no more AST inputs), for some reason I assumed that we would still do some amount of type-related computations on the AST form. (Why did I think this? I guess mostly for lints that want type info... Or will those all be ported to operate on MIR somehow? Doesn't sound very linty at that point)

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Apr 4, 2017

@pnkfelix still a problem you want fixed?

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Apr 6, 2017

I don't know enough our current lints to judge if it is still a problem, so I will close.

@pnkfelix pnkfelix closed this Apr 6, 2017

nivkner added a commit to nivkner/rust that referenced this issue Oct 7, 2017

address more FIXME whose associated issues were marked as closed
update FIXME(rust-lang#6298) to point to open issue 15020
update FIXME(rust-lang#6268) to point to RFC 811
update FIXME(rust-lang#10520) to point to RFC 1751
remove FIXME for emscripten issue 4563 and include target in `test_estimate_scaling_factor`
remove FIXME(rust-lang#18207) since node_id isn't used for `ref` pattern analysis
remove FIXME(rust-lang#6308) since DST was implemented in rust-lang#12938
remove FIXME(rust-lang#2658) since it was decided to not reorganize module
remove FIXME(rust-lang#20590) since it was decided to stay conservative with projection types
remove FIXME(rust-lang#20297) since it was decided that solving the issue is unnecessary
remove FIXME(rust-lang#27086) since closures do correspond to structs now
remove FIXME(rust-lang#13846) and enable `function_sections` for windows
remove mention of rust-lang#22079 in FIXME(rust-lang#22079) since this is a general FIXME
remove FIXME(rust-lang#5074) since the restriction on borrow were lifted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.