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

The Great Generics Generalisation: HIR Followup #51880

Merged
merged 41 commits into from
Aug 20, 2018

Conversation

varkor
Copy link
Member

@varkor varkor commented Jun 28, 2018

Addresses the final comments in #48149.

r? @eddyb, but there are a few things I have yet to clean up. Making the PR now to more easily see when things break.

cc @yodaldevoid

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 28, 2018
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@varkor varkor force-pushed the generics-hir-generalisation-followup branch from 8b45e20 to f82e51f Compare June 28, 2018 21:57
@rust-highfive

This comment has been minimized.


if let Some(ref data) = segments[index].args {
let lifetime_offset = if infer_lifetimes[&index] {
defs.own_counts().lifetimes
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be taken out of the loop.

@varkor varkor force-pushed the generics-hir-generalisation-followup branch 2 times, most recently from 5af5a59 to e72e03d Compare June 29, 2018 22:42
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@varkor varkor force-pushed the generics-hir-generalisation-followup branch from e72e03d to 0d23eef Compare July 2, 2018 00:18
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@varkor varkor force-pushed the generics-hir-generalisation-followup branch 3 times, most recently from 493fcd4 to 0abb1c5 Compare July 2, 2018 17:24
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@@ -25,5 +25,4 @@ fn main() {
//~| ERROR wrong number of type arguments: expected 1, found 0
let _: S<'static +, 'static>;
//~^ ERROR lifetime parameters must be declared prior to type parameters
//~| ERROR at least one non-builtin trait is required for an object type
Copy link
Member Author

Choose a reason for hiding this comment

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

The reason this error is displayed currently is very much dependent on the ad-hoc parameter-separation that currently goes on. Unless it causes more issues, I really don't think it's worth attempting to preserve behaviour here.

@varkor varkor changed the title [WIP] The Great Generics Generalisation: HIR Followup The Great Generics Generalisation: HIR Followup Jul 3, 2018
@varkor
Copy link
Member Author

varkor commented Jul 3, 2018

@eddyb: this is now ready to be reviewed!

@alexreg
Copy link
Contributor

alexreg commented Jul 9, 2018

@varkor @eddyb has been away for a week or so now... might be away a while longer, just to let you know. :-)

})
.cloned()
.collect();
AstConv::prohibit_generics(self, &segs);
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only use of the variable segs? You might want to change prohibit_generics to take an iterator or something. Either way, having a variable here that's not non_generic_segs is confusing.

@stokhos stokhos 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 Jul 21, 2018
@stokhos
Copy link

stokhos commented Jul 21, 2018

Ping from triage, @varkor any update on this PR?

// Provide substitutions for parameters for which (valid) arguments have been provided.
|param, arg| {
match param.kind {
GenericParamDefKind::Lifetime => match arg {

This comment was marked as resolved.

// Provide substitutions for parameters for which (valid) arguments have been provided.
|param, arg| {
match param.kind {
GenericParamDefKind::Lifetime => match arg {

This comment was marked as resolved.

for &PathSeg(def_id, index) in &path_segs {
let seg = &segments[index];
let generics = self.tcx.generics_of(def_id);
// `impl Trait` is treated as a normal generic parameter internally,

This comment was marked as resolved.

This comment was marked as resolved.

if let Some(&PathSeg(_, index)) = path_segs.iter().find(|&PathSeg(did, _)| {
*did == def_id
}) {
// If we've encountered an `impl Trait`-related error, we're just

This comment was marked as resolved.

fn_segment.is_some() as usize;
AstConv::prohibit_generics(self, &segments[..segments.len() - poly_segments]);

let mut generic_segs = HashSet::new();

This comment was marked as resolved.

This comment was marked as resolved.

}
}
infer_types_type_seg = seg.infer_types;
let mut suppress_errors = FxHashMap();

This comment was marked as resolved.

}) {
// If we've encountered an `impl Trait`-related error, we're just
// going to infer the arguments for better error messages.
if !suppress_errors[&index] {

This comment was marked as resolved.

},
// Provide substitutions for parameters for which (valid) arguments have been provided.
|param, arg| {
match param.kind {

This comment was marked as resolved.

|
LL | let _ = S::new::<isize,f64>(1, 1.0);
| ^^^ expected 1 type parameter
| ^^^^^^^^^^^^^^^^^^^ unexpected type argument
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this seems to have regressed, could you somehow use each GenericArg's individual span?

|
LL | let _: S2 = Trait::<'a,isize>::new::<f64>(1, 1.0);
| ^^ expected 0 lifetime parameters
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unexpected lifetime argument
Copy link
Member

Choose a reason for hiding this comment

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

Similar situation here.

@eddyb
Copy link
Member

eddyb commented Aug 20, 2018

Overall I'm really happy with the progress made here! 🎉
The main issue I see before merging this is the diagnostics regression.
Other than that, I won't block this PR on fusing checking and building substs.

@varkor varkor force-pushed the generics-hir-generalisation-followup branch from aa99c17 to 9ba4b1f Compare August 20, 2018 12:07
@@ -255,9 +256,9 @@ impl<'o, 'gcx: 'tcx, 'tcx> dyn AstConv<'gcx, 'tcx>+'o {
&empty_args
},
if is_method_call {
GenericArgPosition::Method
GenericArgPosition::Associated
Copy link
Member

Choose a reason for hiding this comment

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

Okay I was wrong, if it's for the method call syntax, i.e. not Trait::method, then it should called MethodCall.

@varkor varkor force-pushed the generics-hir-generalisation-followup branch from 9ba4b1f to 2d3b1b7 Compare August 20, 2018 12:43
|
LL | fn f<F:Trait(isize) -> isize>(x: F) {}
| ^^^^^^^^^^^^^^^^^^^^^ expected no type arguments
| ^^^^^^^^^^^^^^^^ unexpected type argument
Copy link
Member

Choose a reason for hiding this comment

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

Hmm this span seems wrong, it should end at the ), I wonder if that's a bug in how T(A) -> R turns into T<(A,), Output = R>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this one looked a little weird to me, but I thought it was probably due to function syntax issues that weren't related to this change.

Copy link
Member

Choose a reason for hiding this comment

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

Open an issue so we can maybe clean this up in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #53534.

|
LL | let c: Foo<_, usize> = Foo { r: &5 };
| ^^^^^^^^^^^^^ expected 1 type argument
| ^^^^^ unexpected type argument
Copy link
Member

Choose a reason for hiding this comment

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

Nice improvement!

@varkor varkor force-pushed the generics-hir-generalisation-followup branch from 2d3b1b7 to aa3b5c5 Compare August 20, 2018 15:16
@varkor varkor force-pushed the generics-hir-generalisation-followup branch from b3f9b83 to ee9bd0f Compare August 20, 2018 15:36
@eddyb
Copy link
Member

eddyb commented Aug 20, 2018

@bors r+ p=42 (we don't want this to bitrot)

@bors
Copy link
Contributor

bors commented Aug 20, 2018

📌 Commit ee9bd0f has been approved by eddyb

@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 Aug 20, 2018
@bors
Copy link
Contributor

bors commented Aug 20, 2018

⌛ Testing commit ee9bd0f with merge 1558ae7...

bors added a commit that referenced this pull request Aug 20, 2018
…=eddyb

The Great Generics Generalisation: HIR Followup

Addresses the final comments in #48149.

r? @eddyb, but there are a few things I have yet to clean up. Making the PR now to more easily see when things break.

cc @yodaldevoid
@bors
Copy link
Contributor

bors commented Aug 20, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 1558ae7 to master...

@bors bors merged commit ee9bd0f into rust-lang:master Aug 20, 2018
@varkor varkor deleted the generics-hir-generalisation-followup branch August 20, 2018 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants