make default anon const substs optional #83086

lcnr · 2021-03-13T15:10:03Z

this description is currently outdated

This is the least invasive way I could think of to get a tcx into the super_visit_with impl for consts.

Needed to deal with unused substs for anonymous constants. Note that tcx_for_anon_const_substs isn't actually used anywhere yet. Not sure whether it makes sense to merge this PR without actually implementing the optional const substs 😅

More info can be found in the meeting notes and on zulip

r? @nikomatsakis cc @oli-obk

jackh726

Is this actually used anywhere here? Or are you doing that in a followup PR? I'm just curious if there's a better way. But I think this makes sense and should be fine.

jackh726 · 2021-03-13T15:44:25Z

compiler/rustc_middle/src/ty/fold.rs

+    /// Supplies the `tcx` for an unevaluated anonymous constant in case its default substs
+    /// are not yet supplied.
+    ///
+    /// Visitors which do not look into these substs may leave this unimplemented, so be
+    /// careful when calling this method elsewhere.
+    fn tcx_for_anon_const_substs<'a>(&'a self) -> TyCtxt<'tcx>;


Is there a significant downside to just making this tcx(&self) and always requiring it? Is there any place we have a TypeVisitor but no tcx?

I also think that it's perhaps a bit nice to just have this be tcx and return an Option<TyCtxt<'tcx>>, then were it's used, we can just expect. But given the option, I think it's better to just always require a TyCtxt

the reason I went for tcx_for_anon_const_substs is that I don't want to modify all the type flags stuff, because that would be a far bigger change.

Using Option<TyCtxt<'tcx>> seems ok though I would probably still keep the long name 🤔
Using None incorrectly would cause some fairly bad issues in the future

Why would you have to modify the type flags stuff to just add a function? Also, where are you expecting to use this function?

i want to use this function to get the default anon const substs once these are lazily computed.

making tcx obligatory means that we have to modify stuff like needs_subst to take a TyCtxt, which is a more invasive change and shouldn't be needed.

lcnr · 2021-03-15T22:18:50Z

i now think that merging this PR as is isn't really helpful, going to mark as a draft again and keep pushing the const substs changes to this PR as well 🤔

rust-log-analyzer · 2021-03-15T23:14:04Z

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling libc v0.2.88
   Compiling std v0.0.0 (/checkout/library/std)
   Compiling compiler_builtins v0.1.39
   Compiling unwind v0.0.0 (/checkout/library/unwind)
thread 'rustc' panicked at 'called `Option::unwrap()` on a `None` value', compiler/rustc_middle/src/ty/flags.rs:299:47

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.
note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.52.0-nightly (f229e51bd 2021-03-15) running on x86_64-unknown-linux-gnu

note: compiler flags: -Z macro-backtrace -Z binary-dep-depinfo -Z force-unstable-if-unmarked -C opt-level=3 -C embed-bitcode=no -C codegen-units=1 -C debuginfo=0 -C debug-assertions=on -C overflow-checks=off -C link-args=-Wl,-rpath,$ORIGIN/../lib -C prefer-dynamic -C llvm-args=-import-instr-limit=10 -C embed-bitcode=yes --crate-type lib
note: some of the compiler flags provided by cargo are hidden

query stack during panic:
query stack during panic:
#0 [type_of] computing type of `array::<impl at library/core/src/array/mod.rs:390:9: 394:10>`
#1 [impl_trait_ref] computing trait implemented by `array::<impl at library/core/src/array/mod.rs:390:9: 394:10>`
error: could not compile `core`

To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"

bors · 2021-03-17T01:41:02Z

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

lcnr · 2021-07-08T20:10:25Z

don't have the capacity to work on this rn

…r=nikomatsakis lazily "compute" anon const default substs Continuing the work of rust-lang#83086, this implements the discussed solution for the [unused substs problem](https://github.com/rust-lang/project-const-generics/blob/master/design-docs/anon-const-substs.md#unused-substs). As of now, anonymous constants inherit all of their parents generics, even if they do not use them, e.g. in `fn foo<T, const N: usize>() -> [T; N + 1]`, the array length has `T` as a generic parameter even though it doesn't use it. These *unused substs* cause some backwards incompatible, and imo incorrect behavior, e.g. rust-lang#78369. --- We do not actually filter any generic parameters here and the `default_anon_const_substs` query still a dummy which only checks that - we now prevent the previously existing query cycles and are able to call `predicates_of(parent)` when computing the substs of anonymous constants - the default anon consts substs only include the typeflags we assume it does. Implementing that filtering will be left as future work. --- The idea of this PR is to delay the creation of the anon const substs until after we've computed `predicates_of` for the parent of the anon const. As the predicates of the parent can however contain the anon const we still have to create a `ty::Const` for it. We do this by changing the substs field of `ty::Unevaluated` to an option and modifying accesses to instead call the method `unevaluated.substs(tcx)` which returns the substs as before. If the substs - now `substs_` - of `ty::Unevaluated` are `None`, it means that the anon const currently has its default substs, i.e. the substs it has when first constructed, which are the generic parameters it has available. To be able to call `unevaluated.substs(tcx)` in a `TypeVisitor`, we add the non-defaulted method `fn tcx_for_anon_const_substs(&self) -> Option<TyCtxt<'tcx>>`. In case `tcx_for_anon_const_substs` returns `None`, unknown anon const default substs are skipped entirely. Even when `substs_` is `None` we still have to treat the constant as if it has its default substs. To do this, `TypeFlags` are modified so that it is clear whether they can still change when *exposing* any anon const default substs. A new flag, `HAS_UNKNOWN_DEFAULT_CONST_SUBSTS`, is added in case some default flags are missing. The rest of this PR are some smaller changes to either not cause cycles by trying to access the default anon const substs too early or to be able to access the `tcx` in previously unused locations. cc `@rust-lang/project-const-generics` r? `@nikomatsakis`

lcnr added the A-const-generics Area: const generics (parameters and arguments) label Mar 13, 2021

rust-highfive assigned nikomatsakis Mar 13, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2021

lcnr mentioned this pull request Mar 13, 2021

extract ConstKind::Unevaluated into a struct #83040

Merged

jackh726 requested changes Mar 13, 2021

View reviewed changes

jackh726 assigned jackh726 and unassigned nikomatsakis Mar 13, 2021

lcnr added 3 commits March 13, 2021 17:12

extract ConstKind::Unevaluated into a struct

033eb49

update const_eval_resolve

05e16fc

bless mir-opt tests

78c87fc

lcnr marked this pull request as draft March 15, 2021 22:18

lcnr changed the title ~~require a tcx for TypeVisitor~~ make default anon const substs optional Mar 15, 2021

lcnr added 2 commits March 15, 2021 23:29

require a tcx for TypeVisitor

4e8e894

make unevaluated const substs optional

371c77b

lcnr force-pushed the visitor-tcx branch from 6fc08d5 to 371c77b Compare March 15, 2021 23:05

jackh726 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 Apr 23, 2021

lcnr closed this Jul 8, 2021

lcnr mentioned this pull request Jul 19, 2021

lazily "compute" anon const default substs #87280

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make default anon const substs optional #83086

make default anon const substs optional #83086

lcnr commented Mar 13, 2021 •

edited

Loading

jackh726 left a comment

jackh726 Mar 13, 2021

lcnr Mar 13, 2021

jackh726 Mar 13, 2021

lcnr Mar 15, 2021

lcnr commented Mar 15, 2021

rust-log-analyzer commented Mar 15, 2021

bors commented Mar 17, 2021

lcnr commented Jul 8, 2021

make default anon const substs optional #83086

make default anon const substs optional #83086

Conversation

lcnr commented Mar 13, 2021 • edited Loading

jackh726 left a comment

Choose a reason for hiding this comment

jackh726 Mar 13, 2021

Choose a reason for hiding this comment

lcnr Mar 13, 2021

Choose a reason for hiding this comment

jackh726 Mar 13, 2021

Choose a reason for hiding this comment

lcnr Mar 15, 2021

Choose a reason for hiding this comment

lcnr commented Mar 15, 2021

rust-log-analyzer commented Mar 15, 2021

bors commented Mar 17, 2021

lcnr commented Jul 8, 2021

lcnr commented Mar 13, 2021 •

edited

Loading