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

Change default type parameters to match the behavior specified in the RFC #27761

Closed
wants to merge 7 commits into from

Conversation

Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@jroesch @nikomatsakis @bors @sfackler @steveklabnik
@jroesch
Copy link
Member

jroesch commented Aug 12, 2015

This addresses the issues raised by some members of the community about default priorities, and matches the behavior specified in the RFC.

You can also see the related internals discussion here

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis Aug 12, 2015

src/librustc/middle/infer/type_variable.rs Outdated
.collect()
&TypeVariableValue::Bounded { .. } => Some(ty::TyVid { index: i as u32 }),
&TypeVariableValue::Known(v) => match v.sty {
ty::TyInfer(ty::FloatVar(_)) | ty::TyInfer(ty::IntVar(_)) =>

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Aug 12, 2015

Contributor

Nit: the behavior of this fn merits a comment. Also, ty::TyInfer(_) seems more "forward compatible" to me (or else adding the other cases with a call to bug, since I think they are impossible)
src/librustc_typeck/check/mod.rs Outdated

// We now remove any numeric types that also have defaults, and instead insert
// the type variable with a defined fallback.
for ty in &unsolved_variables {
if let Some(_default) = default_map.get(ty) {

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Aug 12, 2015

Contributor

Nit: I personally prefer is_some() or perhaps Some(_) if your intention is just to check whether it is some.
src/librustc_typeck/check/mod.rs Outdated
// variables without defaults.
// Examine all unsolved variables, and narrow them to the set that have applicable
// defaults. We want to process any unsolved variables that have either an explicit
// user default, literal fallback, or are diverging.
for ty in &unsolved_variables {
let resolved = self.infcx().resolve_type_vars_if_possible(ty);

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Aug 12, 2015

Contributor

So, I think that resolved == ty must always hold at this point in the code. (I can't see why that would not be the case.) If so, we could simplify this code by removing this call, and removing the various matches below.

this is incorrect, because unsolved_variables returns, well, some variables that are in fact solved
src/librustc_typeck/check/mod.rs Outdated
unbound_tyvars.remove(resolved);
}
ty::TyInfer(_) => {
unbound_tyvars.insert(*ty);

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Aug 12, 2015

Contributor

This set unbound_tyvars probably needs a better name, like defaults_to_apply
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Aug 13, 2015

After some discussion over IRC, @jroesch and I agreed current code isn't quite right -- in particular, the reason that user defaults wind up being preferred over integral defaults is because unsolved_variables just happens to return them in that order, which is quite non-obvious. My suggestion is to split the unbound_tyvars set into multiple sets, one for user-defined defaults, one for integral defaults, and one for diverges defaults, and then apply them in that order, making the relative precedence very clear.

We must be sure to apply the user-defined defaults indiscriminantly (whether or not variable has since been bound) because otherwise conflicting defaults will go undetected.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Aug 13, 2015

I also THINK that first loop could be simplified to something like this https://gist.github.com/nikomatsakis/a54823f42f8546436e7a, though naturally we'd want to use distinct sets as discussed in previous comment

@jroesch jroesch force-pushed the jroesch:default-typaram-user-defaults branch 2 times, most recently from 7cefd00 to f9aff1e Aug 19, 2015

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Sep 3, 2015

Tidy is still unhappy. This looks good, modulo the question of what semantics we want. Can we rename things so that all the test cases have some common prefix, like "default-type-param-fallback"?

@jroesch jroesch force-pushed the jroesch:default-typaram-user-defaults branch from 0984c08 to c9ecb8c Sep 4, 2015

@jroesch

This comment has been minimized.

Sign in to view
Copy link
Member Author

jroesch commented Sep 4, 2015

@bors r=nikomatsakis
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Sep 4, 2015

📌 Commit c9ecb8c has been approved by nikomatsakis

@jroesch jroesch force-pushed the jroesch:default-typaram-user-defaults branch from c9ecb8c to 2f5df9e Sep 4, 2015

@jroesch

This comment has been minimized.

Sign in to view
Copy link
Member Author

jroesch commented Sep 4, 2015

@bors r=nikomatsakis
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Sep 4, 2015

📌 Commit 2f5df9e has been approved by nikomatsakis

bors added a commit that referenced this pull request Sep 4, 2015

@bors
Auto merge of #27761 - jroesch:default-typaram-user-defaults, r=nikom… 
…atsakis

This addresses the issues raised by some members of the community about default priorities, and matches the behavior specified in the RFC.

You can also see the related internals discussion [here](https://internals.rust-lang.org/t/interaction-of-user-defined-and-integral-fallbacks-with-inference/2496)

r? @nikomatsakis
b180375
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Sep 4, 2015

⌛️ Testing commit 2f5df9e with merge b180375...
@sfackler

This comment has been minimized.

Sign in to view
Copy link
Member

sfackler commented Sep 4, 2015

Some cfail issues: https://travis-ci.org/rust-lang/rust/builds/78808560#L6472
@jroesch

This comment has been minimized.

Sign in to view
Copy link
Member Author

jroesch commented Sep 4, 2015

@sfackler thanks! looks like I broke the error message formatting on my last commit, will fix and try again
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Sep 4, 2015

💔 Test failed - auto-mac-64-opt
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Sep 15, 2015

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

jroesch added some commits Aug 11, 2015

@jroesch
Change fallback priorties to prefer user defaults
d72fcdf
@jroesch
Refactor default type parameter fallback 
The previous implementation was not very clear, this commit attempts to clarify and clean up the code that
applies all defaults.
c0b7638
@jroesch
Address nits and add assertation
8a93f34
@jroesch
Fix tidy
8b00850
@jroesch
Ensure defaults are normalized after application 
Thanks to @eddyb for bringing this bug to my attention.
fb4134e
@jroesch
Address nits and fix tidy ... again
1302304
@jroesch
Rebase fixes
Loading status checks…
bbcc665

@jroesch jroesch force-pushed the jroesch:default-typaram-user-defaults branch from 2f5df9e to bbcc665 Sep 16, 2015

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 9, 2015

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

This comment has been minimized.

Sign in to view
Copy link
Member

steveklabnik commented Dec 31, 2015

Any updates on this PR?
@jroesch

This comment has been minimized.

Sign in to view
Copy link
Member Author

jroesch commented Dec 31, 2015

@steveklabnik this is my fault, things got a little busy with my move and starting my PhD, Niko was going to rebase this, but I assume he never found time. I'll get in contact with Niko and figure it out.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Jan 29, 2016

Bah, I really ought to rebase this indeed.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Feb 18, 2016

OK, so I pulled a copy of this branch locally and am going to close the PR. I'll follow up with @jroesch privately with a few questions.

@nikomatsakis nikomatsakis closed this Feb 18, 2016

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.