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

Short-cut T: Sized trait selection for ADTs #33138

Merged
merged 12 commits into from
May 6, 2016

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Apr 21, 2016

Basically avoids all nested obligations when checking whether an ADT is sized - this speeds up typeck by ~15%

The refactoring fixed #32963, but I also want to make Copy not object-safe (will commit that soon).

Fixes #33201

r? @nikomatsakis

span_bug!(
obligation.cause.span,
"builtin bound for {:?} was ambig",
"confiriming builtin impl for {:?} where none exists",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/confiriming/confirming/

@arielb1
Copy link
Contributor Author

arielb1 commented Apr 21, 2016

I rationalized the WF behaviour for tuples - this needs a crater run.

@arielb1 arielb1 added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Apr 21, 2016
@nikomatsakis
Copy link
Contributor

crater run launched

@nikomatsakis
Copy link
Contributor

Crater run failed because building this branch failed: https://tools.taskcluster.net/task-inspector/#eqYUkXetSja7miFf4nXmeQ/0

/// such.
/// - a TyError, if a type contained itself. The representability
/// check should catch this case.
fn calculate_sized_constraint_inner(&'tcx self, tcx: &ty::TyCtxt<'tcx>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Total nit: I personally prefer the "main method" to be listed first, and the auxiliary helper methods to be listed below it. I find it easier to read (and indeed I was confused reading this patch for a bit, since the "flow" of code was interrupted by helper fns.) But feel free to ignore me if you would rather keep the current ordering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just rewrote this code about half-a-dozen times.

// - proving `Vec<M>: Front` requires
// - `M: Sized` <-- cycle!
//
// If we skip the normalization step, though, everything goes fine.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add to this comment to say:

"One fix would be lazy normalization. At the moment, we do still eagerly normalize, but we still skip the normalization step because, in PR #33138, we only consider the last field of Vec, which is a usize."

@nikomatsakis
Copy link
Contributor

@arielb1 this is a beautiful patch.

I would r+ it, except that I think it violates my proposed stability policy. I think we ought to be making an effort to either issue warnings or at least issue targeted errors regarding the new changes. Moreover, this seems to be a case where we could readily do so.

I am happy to write up the "breaking change issue description" if you like.

In any case, you should fix the branch so we can do a crater run and test things.

@arielb1
Copy link
Contributor Author

arielb1 commented Apr 22, 2016

@nikomatsakis

Are you talking about the tuple: Sized issue? I don't feel like introducing RFC1214 obligations again. But maybe we need such a framework in general for handling warnings?

@nikomatsakis
Copy link
Contributor

@arielb1 I am talking about the Copy and Sized trait object-safety change but also the tuple: Sized issue, yeah. It seems like we have a specific obligation cause for "tuple-sized" requirements -- we should be able to just issue a warning in that case, no?

@nikomatsakis
Copy link
Contributor

Failing that, we could issue an error, but at least direct people to some place that explains the fix. That may be sufficient, and doesn't require as much precision, since it's ok to direct people unnecessarily.

@nikomatsakis
Copy link
Contributor

(I'd like to see the crater impact; if it is zero or very minimal, issuing an error and trying to direct people is probably adequate.)

@arielb1
Copy link
Contributor Author

arielb1 commented Apr 22, 2016

@nikomatsakis

Reporting a warning would put us in caching hell. I will improve the error message.

@nikomatsakis
Copy link
Contributor

@arielb1 ok, but let's get the crater run going too.

@arielb1
Copy link
Contributor Author

arielb1 commented Apr 22, 2016

@nikomatsakis

Done.

@nikomatsakis
Copy link
Contributor

@arielb1 crater run started

@nikomatsakis
Copy link
Contributor

Crater run results: https://gist.github.com/nikomatsakis/abb0ed492d3cb785af858b87ad3b95fd

  • 4668 crates tested: 3047 working / 1528 broken / 35 regressed / 2 fixed / 56 unknown.

I've not investigated these at all.

@jonas-schievink
Copy link
Contributor

  • shapir is using Box<Error + Sized>
  • All other root regressions are due to trait methods returning Self in a tuple without a Self: Sized bound (I think)

@nikomatsakis
Copy link
Contributor

Tricky. It seems like we can pull that change to tuples out into a distinct PR, and introduce as a warning first -- it may be that we can write some a kind of "warning post-pass" that checks all tuples types that occur in trait methods and issues warnings. This wouldn't cover all possible cases, but hopefully it would hit those that occur in practice (I think they are mostly tied to default methods?). Breaking 34 crates without a warning period seems ungood.

cc @rust-lang/core

@arielb1
Copy link
Contributor Author

arielb1 commented Apr 22, 2016

The problem is that allowing tuples with unsized elements to be sized is too much of a footgun. I guess I should just disallow that.

@bluss
Copy link
Member

bluss commented Apr 22, 2016

@nikomatsakis Fixing rayon cleans up a majority of that list, so it's not so bad. ndarray has been fixed with 0.5.1.

@arielb1
Copy link
Contributor Author

arielb1 commented May 3, 2016

rebased

FutureIncompatibleInfo {
id: LintId::of(UNSIZED_IN_TUPLE),
reference: "RFC PR 1592 <https://github.com/rust-lang/rfcs/pull/1592>",
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add info for OBJECT_UNSAFE_FRAGMENT too?

Also, can you point this at the summary issue that I see you are drafting? (#33242)

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 3, 2016

📌 Commit 2d0fcc9 has been approved by nikomatsakis

@nikomatsakis nikomatsakis removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label May 4, 2016
@arielb1
Copy link
Contributor Author

arielb1 commented May 4, 2016

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented May 4, 2016

📌 Commit ef581db has been approved by nikomatsakis

@arielb1
Copy link
Contributor Author

arielb1 commented May 4, 2016

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented May 4, 2016

📌 Commit 238e4ee has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented May 6, 2016

⌛ Testing commit 238e4ee with merge 5158f3b...

bors added a commit that referenced this pull request May 6, 2016
Short-cut `T: Sized` trait selection for ADTs

Basically avoids all nested obligations when checking whether an ADT is sized - this speeds up typeck by ~15%

The refactoring fixed #32963, but I also want to make `Copy` not object-safe (will commit that soon).

Fixes #33201

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
5 participants