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

Forbid recursive impl trait #56074

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@matthewjasper
Contributor

matthewjasper commented Nov 19, 2018

There is no type T, such that T = [T; 2], but impl Trait could sometimes
be to circumvented this.

This patch makes it a hard error for an opaque type to resolve to such a
"type". Before this can be merged it needs

  • A better error message
  • A crater run (?) to see if this any real-world code
@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 19, 2018

r? @zackmdavis

(rust_highfive has picked a reviewer for you, use r? to override)

@cramertj

This comment has been minimized.

Member

cramertj commented Nov 20, 2018

cc #47659

span: Span,
) {
if let Err(partially_expanded_type) = tcx.try_expand_impl_trait_type(def_id, substs) {
let mut err = tcx.sess.struct_span_err(span, "recursive opaque type");

This comment has been minimized.

@zackmdavis

zackmdavis Nov 22, 2018

Member

Maybe this deserves a dedicated E0XXX error code? (which would be defined in librustc_typeck/diagnostics.rs)

@zackmdavis

This comment has been minimized.

Member

zackmdavis commented Nov 22, 2018

@bors try

@bors

This comment has been minimized.

Contributor

bors commented Nov 22, 2018

⌛️ Trying commit cdf5394 with merge db42d4d...

bors added a commit that referenced this pull request Nov 22, 2018

Auto merge of #56074 - matthewjasper:forbid-recursive-impl-trait, r=<…
…try>

Forbid recursive impl trait

There is no type T, such that `T = [T; 2]`, but impl Trait could sometimes
be to circumvented this.

This patch makes it a hard error for an opaque type to resolve to such a
"type". Before this can be merged it needs

- [ ] A better error message
- [ ] A crater run (?) to see if this any real-world code
@bors

This comment has been minimized.

Contributor

bors commented Nov 22, 2018

☀️ Test successful - status-travis
State: approved= try=True

@matthewjasper

This comment has been minimized.

Contributor

matthewjasper commented Nov 26, 2018

Can I have a crater run @rust-lang/infra?

@pietroalbini

This comment has been minimized.

Member

pietroalbini commented Nov 27, 2018

@craterbot run start=master#0b9f19dff1347e29bf4362ab5a8fab84b43023b5 end=try#db42d4dad33013eba11ef37342ad9f614e5652b8 mode=check-only

@craterbot

This comment has been minimized.

Collaborator

craterbot commented Nov 27, 2018

👌 Experiment pr-56074 created and queued.
🔍 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

This comment has been minimized.

Collaborator

craterbot commented Nov 27, 2018

🚧 Experiment pr-56074 is now running on agent aws-1.

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

@craterbot

This comment has been minimized.

Collaborator

craterbot commented Nov 29, 2018

🎉 Experiment pr-56074 is completed!
📰 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

@pietroalbini

This comment has been minimized.

Member

pietroalbini commented Nov 29, 2018

Only one regression, ThatsGobbles/taggu-rust:

Nov 28 10:49:30.564 INFO [stderr] error: recursive opaque type
Nov 28 10:49:30.564 INFO [stderr]    --> src/metadata/mod.rs:149:63
Nov 28 10:49:30.564 INFO [stderr]     |
Nov 28 10:49:30.564 INFO [stderr] 149 |     pub fn iter_over<'a>(&'a self, mis: MappingIterScheme) -> impl Iterator<Item = &'a String> {
Nov 28 10:49:30.564 INFO [stderr]     |                                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ resolves to self-referential type
Nov 28 10:49:30.564 INFO [stderr]     |
Nov 28 10:49:30.564 INFO [stderr]     = note: resolved type is `generator::GenConverter::gen_to_iter::It<[generator@src/metadata/mod.rs:150:23: 184:10 self:&'a metadata::MetaValue, mis:metadata::MappingIterScheme for<'r, 's, 't0, 't1, 't2, 't3, 't4, 't5, 't6, 't7, 't8, 't9, 't10, 't11, 't12, 't13, 't14, 't15, 't16, 't17, 't18, 't19, 't20, 't21, 't22, 't23, 't24, 't25, 't26, 't27, 't28, 't29, 't30, 't31, 't32, 't33, 't34, 't35> {&'r metadata::MetaValue, metadata::MetaValue, &'s std::string::String, (), &'t0 std::vec::Vec<metadata::MetaValue>, fn(&'t1 std::vec::Vec<metadata::MetaValue>) -> <&'t1 std::vec::Vec<metadata::MetaValue> as std::iter::IntoIterator>::IntoIter {<&'t1 std::vec::Vec<metadata::MetaValue> as std::iter::IntoIterator>::into_iter}, std::slice::Iter<'t2, metadata::MetaValue>, std::slice::Iter<'t3, metadata::MetaValue>, &'t4 metadata::MetaValue, &'t5 metadata::MetaValue, fn(std::boxed::Box<impl std::iter::Iterator>) -> <std::boxed::Box<impl std::iter::Iterator> as std::iter::IntoIterator>::IntoIter {<std::boxed::Box<impl std::iter::Iterator> as std::iter::IntoIterator>::into_iter}, fn(impl std::iter::Iterator) -> std::boxed::Box<impl std::iter::Iterator> {<std::boxed::Box<T>><impl std::iter::Iterator>::new}, &'t8 metadata::MetaValue, metadata::MappingIterScheme, impl std::iter::Iterator, std::boxed::Box<impl std::iter::Iterator>, &'t11 std::string::String, &'t12 std::string::String, std::string::String, &'t13 std::string::String, &'t14 std::collections::BTreeMap<metadata::MetaKey, metadata::MetaValue>, fn(&'t15 std::collections::BTreeMap<metadata::MetaKey, metadata::MetaValue>) -> <&'t15 std::collections::BTreeMap<metadata::MetaKey, metadata::MetaValue> as std::iter::IntoIterator>::IntoIter {<&'t15 std::collections::BTreeMap<metadata::MetaKey, metadata::MetaValue> as std::iter::IntoIterator>::into_iter}, std::collections::btree_map::Iter<'t16, metadata::MetaKey, metadata::MetaValue>, std::collections::btree_map::Iter<'t17, metadata::MetaKey, metadata::MetaValue>, (&'t18 metadata::MetaKey, &'t19 metadata::MetaValue), &'t20 metadata::MetaKey, &'t21 metadata::MetaValue, fn(std::boxed::Box<impl std::iter::Iterator>) -> <std::boxed::Box<impl std::iter::Iterator> as std::iter::IntoIterator>::IntoIter {<std::boxed::Box<impl std::iter::Iterator> as std::iter::IntoIterator>::into_iter}, fn(impl std::iter::Iterator) -> std::boxed::Box<impl std::iter::Iterator> {<std::boxed::Box<T>><impl std::iter::Iterator>::new}, metadata::MetaKey, &'t24 metadata::MetaKey, impl std::iter::Iterator, std::boxed::Box<impl std::iter::Iterator>, &'t27 std::string::String, &'t28 std::string::String, fn(std::boxed::Box<impl std::iter::Iterator>) -> <std::boxed::Box<impl std::iter::Iterator> as std::iter::IntoIterator>::IntoIter {<std::boxed::Box<impl std::iter::Iterator> as std::iter::IntoIterator>::into_iter}, fn(impl std::iter::Iterator) -> std::boxed::Box<impl std::iter::Iterator> {<std::boxed::Box<T>><impl std::iter::Iterator>::new}, &'t31 metadata::MetaValue, impl std::iter::Iterator, std::boxed::Box<impl std::iter::Iterator>, &'t34 std::string::String, &'t35 std::string::String}]>`
@matthewjasper

This comment has been minimized.

Contributor

matthewjasper commented Nov 29, 2018

Regression is due to this change. Code in question is:

impl MetaValue {
    pub fn iter_over<'a>(&'a self, mis: MappingIterScheme) -> impl Iterator<Item = &'a String> {
        let closure = move || {
            match *self {
                MetaValue::Nil => {},
                MetaValue::Str(ref s) => { yield s; },
                MetaValue::Seq(ref mvs) => {
                    for mv in mvs {
                        for i in Box::new(mv.iter_over(mis)) {
                            yield i;
                        }
                    }
                },
                MetaValue::Map(ref map) => {
                    for (mk, mv) in map {
                        match mis {
                            MappingIterScheme::Keys | MappingIterScheme::Both => {
                                // This outputs the value of the Nil key first, but only if a BTreeMap is used.
                                for s in Box::new(mk.iter_over()) {
                                    yield s;
                                }
                            },
                            MappingIterScheme::Vals => {},
                        };

                        match mis {
                            MappingIterScheme::Vals | MappingIterScheme::Both => {
                                for s in Box::new(mv.iter_over(mis)) {
                                    yield s;
                                }
                            },
                            MappingIterScheme::Keys => {},
                        };
                    }
                },
            }
        };

        GenConverter::gen_to_iter(closure)
    }
}

Replacing for s in Box::new(mv.iter_over(mis)) { with

let iter = Box::new(mv.iter_over(mis)) as Box<dyn Iterator<Item = &'a String>>;
for s in iter {

fixes this.
using an existential type with a wrapper struct should also work at some point.

So @rust-lang/lang, should this be a future compatibility lint (with what default level)?

@eddyb

This comment has been minimized.

Member

eddyb commented Dec 2, 2018

@nikomatsakis

the code is r=me-- haven't looked or thought about crater run results yet

use crate::ty::fold::TypeFolder;
struct OpaqueTypeExpander<'a, 'gcx, 'tcx> {
seen_opaque_tys: FxHashSet<DefId>,

This comment has been minimized.

@nikomatsakis

nikomatsakis Dec 6, 2018

Contributor

Nit: I found this hashset kind of confusing. I see now that it is modeling a stack, since we remove things from it, but I think this merits at least a comment (or perhaps just use a Vec and do a linear search). Something like:

/// Contains the def-ids of opaque types that are actively being
/// expanded. We insert a def-id when we begin expanding an
/// opaque type and remove it once we finish.
check_bounds_are_used(tcx, &generics, pty_ty);
let substs = Substs::identity_for_item(tcx, def_id);
check_opaque(tcx, def_id, substs, it.span);

This comment has been minimized.

@nikomatsakis

nikomatsakis Dec 6, 2018

Contributor

It kind of surprised me to see this check here, but I guess it all works out because of the on-demand business.

self.tcx
}
fn fold_ty(&mut self, t: Ty<'tcx>) -> Ty<'tcx> {

This comment has been minimized.

@nikomatsakis

nikomatsakis Dec 6, 2018

Contributor

One thought: Maybe we should just use visit_ty instead? Seems more efficient, and wouldn't have to return -> Ty<'tcx>?

This comment has been minimized.

@matthewjasper

matthewjasper Dec 15, 2018

Contributor

There still needs to be a type folder for diagnostics, and I'm not sure how much would be saved by only running a visitor at first.

@Centril Centril removed the I-nominated label Dec 13, 2018

@matthewjasper

This comment has been minimized.

Contributor

matthewjasper commented Dec 15, 2018

@Centril was a decision reached on whether this should be a future compat lint?

@Centril

This comment has been minimized.

Contributor

Centril commented Dec 16, 2018

I don't recall a decision on that but as there was a regression you might as well do it. An issue filed against the regressed repo would also be nice.

matthewjasper added some commits Nov 14, 2018

Display `impl Sized` correctly
It used to display as just `impl`
Forbid impl Trait from referring to unnamable recursive types
There is no type T, such that `T = [T; 2]`, we should not allow this
to be circumvented by impl Trait.

@matthewjasper matthewjasper force-pushed the matthewjasper:forbid-recursive-impl-trait branch from cdf5394 to d550e15 Dec 16, 2018

@matthewjasper

This comment has been minimized.

Contributor

matthewjasper commented Dec 16, 2018

When trying to create an issue for the lint, I couldn't find an example that compiles on stable as they require generators. Given this I've just kept it a hard error and given it an error code explanation.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 16, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:1e8f4012:start=1545000892138495185,finish=1545000893206188224,duration=1067693039
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---

[00:03:19] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:20] tidy error: duplicate error code: 717
[00:03:20] tidy error: /checkout/src/libsyntax/diagnostic_list.rs:416:     E0717, // rustc_promotable without stability attribute
[00:03:20] tidy error: /checkout/src/librustc_typeck/diagnostics.rs:4830: E0717: r##"
[00:03:21] some tidy checks failed
[00:03:21] 
[00:03:21] 
[00:03:21] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:21] 
[00:03:21] 
[00:03:21] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:21] Build completed unsuccessfully in 0:00:48
[00:03:21] Build completed unsuccessfully in 0:00:48
[00:03:21] Makefile:79: recipe for target 'tidy' failed
[00:03:21] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:12e2b891
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun Dec 16 22:58:23 UTC 2018
---
travis_time:end:019253fa:start=1545001103889589801,finish=1545001103895967278,duration=6377477
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:08948f5b
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:10a234e1
travis_time:start:10a234e1
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:09efa14a
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment