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

instantiate_value_path: on SelfCtor, avoid unconstrained tyvars #69340

Merged
merged 1 commit into from
Feb 28, 2020

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Feb 21, 2020

Fixes #69306.

On Self(...) (that is, a Res::SelfCtor), do not use self.impl_self_ty(...). The problem with that method is that it creates unconstrained inference variables for type parameters in the impl (e.g. impl<T> S0<T>). These variables then eventually get substituted for something else when they come in contact with the expected type (e.g. S0<u8>) or merely the arguments passed to the tuple constructor (e.g. the 0 in Self(0)).

Instead of using self.impl_self_ty(...), we instead merely use let ty = self.normalize_ty(span, tcx.at(span).type_of(impl_def_id)); to get the rewritten res.

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 21, 2020
@Centril Centril added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 21, 2020
@eddyb
Copy link
Member

eddyb commented Feb 21, 2020

... do not use self.impl_self_ty ...
The problem with that method is that it creates unconstrained inference variables for type parameters in the impl

Wait what? That's not what the name would suggest... If it creates inference variables it should have instantiate in the name IMO.

@@ -5465,9 +5465,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
.unwrap_or(false);

let (res, self_ctor_substs) = if let Res::SelfCtor(impl_def_id) = res {
let ty = self.impl_self_ty(span, impl_def_id).ty;
Copy link
Member

@eddyb eddyb Feb 21, 2020

Choose a reason for hiding this comment

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

Okay, I see, the .ty should've been a big warning flag (the other field is... substs).

Can you also change the call sites in check::method::suggest to just use tcx.type_of(impl_def_id)?
(EDIT: okay that wouldn't do the same thing, but I'm curious if that's necessary or if it actually makes things worse?)

And then maybe inline impl_self_ty into the only remaining use site (which is in check::method::confirm?).

If impl_self_ty was used more widely in check::method I'd suggest renaming it to instantiate_impl_self_ty and make it private to check::method. But it's not, so might as well get rid of it before this happens again.

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 would prefer not dealing with it in this PR. I've filed #69489 for the follow-up.

@eddyb
Copy link
Member

eddyb commented Feb 21, 2020

@bors try (I believe this deserves a check-only crater run)

@bors
Copy link
Contributor

bors commented Feb 21, 2020

⌛ Trying commit 6cdb2d92aa973eaf4e84f5bf1846f0dba0540e1c with merge 9ff4d07208097950831ed4ea9d76feb1eafc8baa...

@Centril
Copy link
Contributor Author

Centril commented Feb 21, 2020

Note to self: Investigate whether this should be self.normalize_ty(tcx.type_of(...)) so that if the Self type is a projection or some such, we normalize it before seeing whether the normalized type is an ADT. (Also add a test case.)

EDIT: fixed.

@bors
Copy link
Contributor

bors commented Feb 21, 2020

☀️ Try build successful - checks-azure
Build commit: 9ff4d07208097950831ed4ea9d76feb1eafc8baa (9ff4d07208097950831ed4ea9d76feb1eafc8baa)

@Centril
Copy link
Contributor Author

Centril commented Feb 21, 2020

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-69340 created and queued.
🤖 Automatically detected try build 9ff4d07208097950831ed4ea9d76feb1eafc8baa
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2020
@craterbot
Copy link
Collaborator

🚧 Experiment pr-69340 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-69340 is completed!
📊 21 regressed and 1 fixed (90999 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@Centril
Copy link
Contributor Author

Centril commented Feb 24, 2020

Crater triage:

  • There are 6 root regressions.
    • 1 was in my own crate and has already been fixed (with a point release).
    • 4 has already merged regression fixes.
    • The 1 remaining has a PR open fixing the problem.

@Centril
Copy link
Contributor Author

Centril commented Feb 26, 2020

I added a call to normalize_ty as well to handle projections, as this was previously allowed, and is allowed when using Self { ... }.

I believe this should be ready to go as there's a single remaining regression which has an open PR.
r? @petrochenkov

@pnkfelix
Copy link
Member

pnkfelix commented Feb 27, 2020

discussed at T-compiler meeting. Delaying question of whether to beta backport to next week's meeting.

@nikomatsakis
Copy link
Contributor

@bors r+

My two cents:

The change would be difficult, from a technical perspective, to do a future-compatibility warning for.

The impact is non-trivial but manageable, and @Centril did a great job of opening PRs (which have been merged). However, this doesn't mean we're "all set", as those changes still have to make their way into new releases (ideally, point releases), and that usually takes some time.

I'm inclined to approve this, but I think we want to be on the lookout for bug reports from folks that are affected "in the wild".

I'm somewhat inclined not to backport, so as to give people a bit more time to adapt before it reaches a stable release, but it depends on how many regression reports we receive after landing.

@bors
Copy link
Contributor

bors commented Feb 27, 2020

📌 Commit 6672a04 has been approved by nikomatsakis

@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 Feb 27, 2020
@Centril
Copy link
Contributor Author

Centril commented Feb 27, 2020

r? @nikomatsakis

bors added a commit that referenced this pull request Feb 28, 2020
Rollup of 10 pull requests

Successful merges:

 - #68989 (Update RELEASES.md for 1.42.0)
 - #69340 (instantiate_value_path: on `SelfCtor`, avoid unconstrained tyvars)
 - #69384 (parser: `token` -> `normalized_token`, `nonnormalized_token` -> `token`)
 - #69452 (typeck: use `Pattern` obligation cause more for better diagnostics)
 - #69481 (use char instead of &str for single char patterns)
 - #69522 (error_derive_forbidden_on_non_adt: be more graceful)
 - #69538 (Stabilize `boxed_slice_try_from`)
 - #69539 (late resolve, visit_fn: bail early if there's no body.)
 - #69541 (Remove unneeded calls to format!())
 - #69547 (remove redundant clones, references to operands, explicit boolean comparisons and filter(x).next() calls.)

Failed merges:

r? @ghost
@bors bors merged commit 76fe449 into rust-lang:master Feb 28, 2020
@Centril Centril deleted the self-ctor-normalize branch February 28, 2020 21:21
@pnkfelix
Copy link
Member

pnkfelix commented Mar 5, 2020

discussed in T-compiler meeting. declined for beta-backport.

@pnkfelix pnkfelix removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 5, 2020
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 16, 2020
Pkgsrc changes:
 * Bump rust bootstrap version to 1.42.0, except for Darwin/i686 where the
   bootstrap is not (yet?) available.

Upstream changes:

Version 1.43.0 (2020-04-23)
==========================

Language
--------
- [Fixed using binary operations with `&{number}` (e.g. `&1.0`) not having
  the type inferred correctly.][68129]
- [Attributes such as `#[cfg()]` can now be used on `if` expressions.][69201]

**Syntax only changes**
- [Allow `type Foo: Ord` syntactically.][69361]
- [Fuse associated and extern items up to defaultness.][69194]
- [Syntactically allow `self` in all `fn` contexts.][68764]
- [Merge `fn` syntax + cleanup item parsing.][68728]
- [`item` macro fragments can be interpolated into `trait`s, `impl`s,
  and `extern` blocks.][69366]
  For example, you may now write:
  ```rust
  macro_rules! mac_trait {
      ($i:item) => {
          trait T { $i }
      }
  }
  mac_trait! {
      fn foo() {}
  }
  ```
These are still rejected *semantically*, so you will likely receive an error but
these changes can be seen and parsed by macros and
conditional compilation.


Compiler
--------
- [You can now pass multiple lint flags to rustc to override the previous
  flags.][67885] For example; `rustc -D unused -A unused-variables` denies
  everything in the `unused` lint group except `unused-variables` which
  is explicitly allowed. However, passing `rustc -A unused-variables -D unused` denies
  everything in the `unused` lint group **including** `unused-variables` since
  the allow flag is specified before the deny flag (and therefore overridden).
- [rustc will now prefer your system MinGW libraries over its bundled libraries
  if they are available on `windows-gnu`.][67429]
- [rustc now buffers errors/warnings printed in JSON.][69227]

Libraries
---------
- [`Arc<[T; N]>`, `Box<[T; N]>`, and `Rc<[T; N]>`, now implement
  `TryFrom<Arc<[T]>>`,`TryFrom<Box<[T]>>`, and `TryFrom<Rc<[T]>>`
  respectively.][69538] **Note** These conversions are only available when `N`
  is `0..=32`.
- [You can now use associated constants on floats and integers directly, rather
  than having to import the module.][68952] e.g. You can now write `u32::MAX` or
  `f32::NAN` with no imports.
- [`u8::is_ascii` is now `const`.][68984]
- [`String` now implements `AsMut<str>`.][68742]
- [Added the `primitive` module to `std` and `core`.][67637] This module
  reexports Rust's primitive types. This is mainly useful in macros
  where you want avoid these types being shadowed.
- [Relaxed some of the trait bounds on `HashMap` and `HashSet`.][67642]
- [`string::FromUtf8Error` now implements `Clone + Eq`.][68738]

Stabilized APIs
---------------
- [`Once::is_completed`]
- [`f32::LOG10_2`]
- [`f32::LOG2_10`]
- [`f64::LOG10_2`]
- [`f64::LOG2_10`]
- [`iter::once_with`]

Cargo
-----
- [You can now set config `[profile]`s in your `.cargo/config`, or through
  your environment.][cargo/7823]
- [Cargo will now set `CARGO_BIN_EXE_<name>` pointing to a binary's
  executable path when running integration tests or benchmarks.][cargo/7697]
  `<name>` is the name of your binary as-is e.g. If you wanted the executable
  path for a binary named `my-program`you would use
  `env!("CARGO_BIN_EXE_my-program")`.

Misc
----
- [Certain checks in the `const_err` lint were deemed unrelated to const
  evaluation][69185], and have been moved to the `unconditional_panic` and
  `arithmetic_overflow` lints.

Compatibility Notes
-------------------

- [Having trailing syntax in the `assert!` macro is now a hard error.][69548]
  This has been a warning since 1.36.0.
- [Fixed `Self` not having the correctly inferred type.][69340] This incorrectly
  led to some instances being accepted, and now correctly emits a hard error.

[69340]: rust-lang/rust#69340

Internal Only
-------------
These changes provide no direct user facing benefits, but represent significant
improvements to the internals and overall performance of `rustc` and
related tools.

- [All components are now built with `opt-level=3` instead of `2`.][67878]
- [Improved how rustc generates drop code.][67332]
- [Improved performance from `#[inline]`-ing certain hot functions.][69256]
- [traits: preallocate 2 Vecs of known initial size][69022]
- [Avoid exponential behaviour when relating types][68772]
- [Skip `Drop` terminators for enum variants without drop glue][68943]
- [Improve performance of coherence checks][68966]
- [Deduplicate types in the generator witness][68672]
- [Invert control in struct_lint_level.][68725]

[67332]: rust-lang/rust#67332
[67429]: rust-lang/rust#67429
[67637]: rust-lang/rust#67637
[67642]: rust-lang/rust#67642
[67878]: rust-lang/rust#67878
[67885]: rust-lang/rust#67885
[68129]: rust-lang/rust#68129
[68672]: rust-lang/rust#68672
[68725]: rust-lang/rust#68725
[68728]: rust-lang/rust#68728
[68738]: rust-lang/rust#68738
[68742]: rust-lang/rust#68742
[68764]: rust-lang/rust#68764
[68772]: rust-lang/rust#68772
[68943]: rust-lang/rust#68943
[68952]: rust-lang/rust#68952
[68966]: rust-lang/rust#68966
[68984]: rust-lang/rust#68984
[69022]: rust-lang/rust#69022
[69185]: rust-lang/rust#69185
[69194]: rust-lang/rust#69194
[69201]: rust-lang/rust#69201
[69227]: rust-lang/rust#69227
[69548]: rust-lang/rust#69548
[69256]: rust-lang/rust#69256
[69361]: rust-lang/rust#69361
[69366]: rust-lang/rust#69366
[69538]: rust-lang/rust#69538
[cargo/7823]: rust-lang/cargo#7823
[cargo/7697]: rust-lang/cargo#7697
[`Once::is_completed`]: https://doc.rust-lang.org/std/sync/struct.Once.html#method.is_completed
[`f32::LOG10_2`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG10_2.html
[`f32::LOG2_10`]: https://doc.rust-lang.org/std/f32/consts/constant.LOG2_10.html
[`f64::LOG10_2`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG10_2.html
[`f64::LOG2_10`]: https://doc.rust-lang.org/std/f64/consts/constant.LOG2_10.html
[`iter::once_with`]: https://doc.rust-lang.org/std/iter/fn.once_with.html
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent Self behavior with generic structs
8 participants