Skip to content

/rust

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

Some more pattern cleanup and bugfixing #34365

Merged
merged 11 commits into from Jul 10, 2016

Conversation

Reviewers
No reviews
Assignees

@eddyb eddyb

Labels
T-lang
Projects
None yet
Milestone
No milestone
6 participants
@petrochenkov @eddyb @bors @nikomatsakis @aturon @pnkfelix
@petrochenkov
Copy link
Contributor

petrochenkov commented Jun 19, 2016 •
edited

The next part of #34095

The most significant fixed mistake is definitions for partially resolved associated types not being updated after full resolution.

fn f<T: Fn()>(arg: T::Output) { .... } // <- the definition of T::Output was not updated in def_map

For this reason unstable associated types of stable traits, like FnOnce::Output, could be used in stable code when written in unqualified form. Now they are properly checked, this is a [breaking-change] (pretty minor one, but a crater run would be nice). The fix is not to use unstable library features in stable code, alternatively FnOnce::Output can be stabilized.

Besides that, paths in struct patterns and expressions S::A { .. } are now fully resolved as associated types. Such types cannot be identified as structs at the moment, i.e. the change doesn't make previously invalid code valid, but it improves error diagnostics.

Other changes: Def::Err is supported better (less chances for ICEs for erroneous code), some incorrect error messages are corrected, some duplicated error messages are not reported, ADT definitions are now available through constructor IDs, everything else is cleanup and code audit.

Fixes #34209
Closes #22933 (adds tests)

r? @eddyb

@rust-highfive rust-highfive assigned eddyb Jun 19, 2016

@petrochenkov petrochenkov force-pushed the petrochenkov:deferr branch from 0dcd412 to 914bf7d Jun 19, 2016

@eddyb eddyb added the S-waiting-on-crater label Jun 19, 2016

@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Jun 19, 2016

cc @nrc @nikomatsakis
src/librustc_typeck/collect.rs
let adt = ccx.tcx.intern_adt_def(did, ty::AdtKind::Struct, variants);
if let Some(ctor_id) = ctor_id {
// Make adt definition available through constructor id as well.
ccx.tcx.insert_adt_def(ctor_id, adt);

This comment has been minimized.

Sign in to view
Copy link
@eddyb

eddyb Jun 19, 2016

Member

Why was this done?

This comment has been minimized.

Sign in to view
Copy link
@petrochenkov

petrochenkov Jun 19, 2016 •
edited

Author Contributor

So you can go directly from Def::Struct to ADT definition regardless of where this Def::Struct is used - in a pattern, expression or a type. Sometimes Def::Struct denotes a type and sometimes a constructor and previously lookup_adt_def worked only with Def::Structs in type context and panicked otherwise, so workarounds had to be used.

@petrochenkov petrochenkov referenced this pull request Jun 19, 2016

Merged

Merge PatKind::QPath into PatKind::Path in AST #34368

src/librustc_typeck/check/mod.rs
let ty_substituted = self.instantiate_type_scheme(path.span, substs, &type_scheme.ty);
self.write_ty(node_id, ty_substituted);
self.write_substs(node_id, ty::ItemSubsts {
substs: substs

This comment has been minimized.

Sign in to view
Copy link
@petrochenkov

petrochenkov Jun 21, 2016

Author Contributor

Hm, this is probably a mistake and regression. The substs written are substs before instantiate_type_scheme, not after.
I.e. for

type Alias<T> = S<T, u16>;
// Inside match
Alias::<u8> { .. }

only u8 is written, not [u8, u16].
However, instantiate_path does the same mistake(?) even without this PR.

@petrochenkov petrochenkov referenced this pull request Jun 22, 2016

Closed

Rustc panic when wrong namespace in destructuring patern #34413

jseyfried added a commit to jseyfried/rust that referenced this pull request Jun 25, 2016

@jseyfried
Rollup merge of rust-lang#34368 - petrochenkov:astqpath, r=Manishearth 
The AST part of rust-lang#34365

plugin-[breaking-change] cc rust-lang#31645
7cf6761

jseyfried added a commit to jseyfried/rust that referenced this pull request Jun 25, 2016

@jseyfried
Rollup merge of rust-lang#34368 - petrochenkov:astqpath, r=Manishearth 
The AST part of rust-lang#34365

plugin-[breaking-change] cc rust-lang#31645
4e2e31c
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 28, 2016

☔️ The latest upstream changes (presumably #34424) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov petrochenkov force-pushed the petrochenkov:deferr branch from 914bf7d to 6bd1d7a Jun 29, 2016

@petrochenkov

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

petrochenkov commented Jun 29, 2016

Rebased.
Still waiting for crater run.
@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Jun 29, 2016

@petrochenkov Sorry about that, @nikomatsakis is on vacation.
But I have crater access now, so here's the crater report.
The two actual root regressions are in traverse and parsell, both using FnOnce::Output.

@eddyb eddyb added I-nominated T-lang and removed S-waiting-on-crater labels Jun 29, 2016

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jun 29, 2016

So as I wrote on IRC, I don't think that F::Output (where F: FnOnce) has to be unstable -- and I'm not sure it was intended to be, though I'll have to check around. What we wanted to be careful about was something like F: FnOnce<()>, because this reveals the form of the "input types", and there was some concern that F: FnOnce<(A,B)> might change to F: FnOnce<A,B> or something, depending on what approach we wound up with for variadic generics. But I think we know that the return type will always be an associated type, and I see no reason we would ever change its name.

@petrochenkov petrochenkov referenced this pull request Jun 29, 2016

Closed

Non-existent struct enum variant in pattern causes ICE #34564

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jun 30, 2016

☔️ The latest upstream changes (presumably #34552) made this pull request unmergeable. Please resolve the merge conflicts.
@aturon

This comment has been minimized.

Sign in to view
Copy link
Member

aturon commented Jun 30, 2016

But I think we know that the return type will always be an associated type, and I see no reason we would ever change its name.

I agree on both counts. It seems OK for this name to be available on stable Rust.
@aturon

This comment has been minimized.

Sign in to view
Copy link
Member

aturon commented Jul 7, 2016

Lang team meeting: we decided we should try this PR, but with marking the Output type as stable, and see whether that eliminates the regressions.
@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Jul 7, 2016

@petrochenkov Looks like FnOnce::Output is explicitly marked unstable so just flipping that should work. This is likely what @nikomatsakis had in mind.

@pnkfelix pnkfelix removed the I-nominated label Jul 7, 2016

petrochenkov added some commits Jun 11, 2016

@petrochenkov
Remove unnecessary accessor function VariantDefData::kind
49ea3d4
@petrochenkov
Merge PatKind::QPath into PatKind::Path in HIR
9c05fb2
@petrochenkov
Resolve partially resolved paths in struct patterns/expressions 
Treat Def::Err correctly in struct patterns
Make instantiate_path and instantiate_type a bit closer to each other
a397b60
@petrochenkov
Do not generate Def::Err in bindings 
Instead of Def::Err erroneous bindings can get usual definitions that doesn't require special cases later on and have less chances to generate ICE.
eb32440
@petrochenkov
Stabilize `FnOnce::Output` + Fix rebase
Loading status checks…
d27e55c

@petrochenkov petrochenkov force-pushed the petrochenkov:deferr branch from 6bd1d7a to d27e55c Jul 8, 2016

@petrochenkov

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

petrochenkov commented Jul 8, 2016

Rebased, FnOnce::Output is stabilized.
@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Jul 9, 2016

Crater report only shows 4 regressions in code using the unstable librustc APIs.

@alexcrichton @aturon Can we merge this with the stabilization of FnOnce::Output directly?
@aturon

This comment has been minimized.

Sign in to view
Copy link
Member

aturon commented Jul 9, 2016

@eddyb Yep, at this point everybody's signed off on that step.
@eddyb

This comment has been minimized.

Sign in to view
Copy link
Member

eddyb commented Jul 9, 2016

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 9, 2016

📌 Commit d27e55c has been approved by eddyb
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 9, 2016

⌛️ Testing commit d27e55c with merge f93aaf8...

bors added a commit that referenced this pull request Jul 9, 2016

@bors
Auto merge of #34365 - petrochenkov:deferr, r=eddyb 
Some more pattern cleanup and bugfixing

The next part of #34095

The most significant fixed mistake is definitions for partially resolved associated types not being updated after full resolution.
```
fn f<T: Fn()>(arg: T::Output) { .... } // <- the definition of T::Output was not updated in def_map
```
For this reason unstable associated types of stable traits, like `FnOnce::Output`, could be used in stable code when written in unqualified form. Now they are properly checked, this is a **[breaking-change]** (pretty minor one, but a crater run would be nice). The fix is not to use unstable library features in stable code, alternatively `FnOnce::Output` can be stabilized.

Besides that, paths in struct patterns and expressions `S::A { .. }` are now fully resolved as associated types. Such types cannot be identified as structs at the moment, i.e. the change doesn't make previously invalid code valid, but it improves error diagnostics.

Other changes: `Def::Err` is supported better (less chances for ICEs for erroneous code), some incorrect error messages are corrected, some duplicated error messages are not reported, ADT definitions are now available through constructor IDs, everything else is cleanup and code audit.

Fixes #34209
Closes #22933 (adds tests)

r? @eddyb
f93aaf8
@bors

@bors bors merged commit d27e55c into rust-lang:master Jul 10, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

This was referenced Jul 10, 2016

Closed
Merged
Merged

@petrochenkov petrochenkov referenced this pull request Jul 12, 2016

Open

Tracking issue for Fn traits (`unboxed_closures` feature) #29625

0 of 2 tasks complete

@petrochenkov petrochenkov deleted the petrochenkov:deferr branch Sep 21, 2016

@petrochenkov petrochenkov restored the petrochenkov:deferr branch Sep 21, 2016

@petrochenkov petrochenkov deleted the petrochenkov:deferr branch Sep 21, 2016

@TimNN TimNN referenced this pull request Oct 7, 2016

Closed

“Let NullaryConstructor = something;” causes internal compiler error: “tried to overwrite interned AdtDef” #37026

@petrochenkov petrochenkov referenced this pull request Oct 24, 2016

Merged

Support `Self` in struct expressions and patterns #37035

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.