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

permit negative impls for non-auto traits #68004

Open
wants to merge 3 commits into
base: master
from

Conversation

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 8, 2020

This is a prototype impl that extends impl !Trait beyond auto traits. It is not integrated with coherence or anything else, and hence only serves to prevent downstream impls (but not to allow downstream crates to rely on the absence of such impls for coherence purposes).

Fixes #66544

TODO:

  • need a test that you can't rely on negative impls for coherence purposes
  • test that negative impls cannot specialize positive ones
  • test that positive impls cannot specialize negative ones
  • extend negative impl to Clone in order to fully fix #66544
  • and maybe make CoerceUnsized unsafe? -- that problem is now split out into #68015
  • introduce feature flag and prepare a write-up
  • improve diagnostics?
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 8, 2020

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 8, 2020

r? @varkor

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

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Jan 8, 2020

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, 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.
2020-01-08T11:45:26.7405974Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-08T11:45:26.7419311Z ##[command]git config gc.auto 0
2020-01-08T11:45:26.7425493Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-08T11:45:26.7455963Z ##[command]git config --get-all http.proxy
2020-01-08T11:45:26.7459002Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/68004/merge:refs/remotes/pull/68004/merge

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)

@Aaron1011

This comment has been minimized.

Copy link
Contributor

Aaron1011 commented Jan 8, 2020

@nikomatsakis: How does this relate to reservation impls?

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Jan 8, 2020

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, 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.
2020-01-08T12:56:26.3794362Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-08T12:56:26.3944156Z ##[command]git config gc.auto 0
2020-01-08T12:56:26.4025198Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-08T12:56:26.4084475Z ##[command]git config --get-all http.proxy
2020-01-08T12:56:26.4232419Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/68004/merge:refs/remotes/pull/68004/merge

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)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 8, 2020

A negative impl is a semver commitment never to implement the given trait for the given type. It should eventually mean that you can rely on the impl not existing for the purposes of negative reasoning in coherence, though this PR doesn't implement that. What this PR does is simply require that no negative and positive impls (for the same trait) can overlap. This means that you can rely on an impl not existing for the purpose of proving soundness, and in particular for proving Pin sound.

A reservation impl is...something harder to express. It is considered a kind of "ambiguous" impl -- it doesn't exist yet, but it might exist in the future. This prevents people from assuming negatively that it would never exist. But it doesn't prevent you from writing overlapping impls.

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:negative-impls branch from e3a0b47 to e16b360 Jan 8, 2020
@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Jan 8, 2020

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, 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.
2020-01-08T13:36:41.7959026Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-08T13:36:41.8219578Z ##[command]git config gc.auto 0
2020-01-08T13:36:41.8223714Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-08T13:36:41.8225585Z ##[command]git config --get-all http.proxy
2020-01-08T13:36:41.8319420Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/68004/merge:refs/remotes/pull/68004/merge

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)

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:negative-impls branch from e16b360 to c6875fd Jan 8, 2020
@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 8, 2020

@comex do you think you could provide me with "clean, minimized versions" of your exploits? The code in this playground is rather complex .. I'd prefer to have three or four simple tests I can add for regression tests. They don't have to be fully weaponized necessarily, either -- something like what @withoutboats did would be perfect.

(In particular, I believe we need a negative Clone impl for &mut T, at least, before this particular PR is done. I guess we also need to update the CoerceUnsized trait to be unsafe for us to fully fix the "rogue impl" problem? That feels like maybe a separate PR, but maybe I'll do it together.)

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:negative-impls branch from c6875fd to e2a82fa Jan 8, 2020
@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 8, 2020

@nikomatsakis I can provide minimal examples for both DerefMut and Clone. Here they are:

DerefMut for shared reference: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7d0c6cef76960efecd8e9afd3a851443

Clone for mutable reference: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=55430b595a04fa05c52fb6abe024d5e8

(You'll notice these work almost exactly the same way.)

For CoerceUnsized I don't have a sample ready, but I think fixing CoerceUnsized should be tracked separately, possibly even opening a separate issue from #66544

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 8, 2020

@withoutboats

For CoerceUnsized I don't have a sample ready, but I think fixing CoerceUnsized should be tracked separately, possibly even opening a separate issue from #66544

I was thinking that might be a good idea as well, as the description of #66544 doesn't seem to match.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 8, 2020

Yea I think it'd be better to track the coerceunsized problem as "we added an unchecked invariant to implementing coerceunsized but didnt mark the trait unsafe," just separate from this coherence hole issue.

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Jan 8, 2020

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, 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.
2020-01-08T14:03:20.7419889Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-08T14:03:20.7508444Z ##[command]git config gc.auto 0
2020-01-08T14:03:20.7596598Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-08T14:03:20.7649570Z ##[command]git config --get-all http.proxy
2020-01-08T14:03:20.7809094Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/68004/merge:refs/remotes/pull/68004/merge
---
2020-01-08T14:17:39.4618507Z    Compiling serde_json v1.0.40
2020-01-08T14:17:41.1285790Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2020-01-08T14:17:51.0477387Z     Finished release [optimized] target(s) in 1m 22s
2020-01-08T14:17:51.0588204Z tidy check
2020-01-08T14:17:51.3581595Z tidy error: /checkout/src/librustc_error_codes/error_codes/E0746.md: too many trailing newlines (3)
2020-01-08T14:17:53.9275051Z Found 488 error codes
2020-01-08T14:17:53.9275375Z Found 1 error codes with no tests
2020-01-08T14:17:53.9275609Z Done!
2020-01-08T14:17:53.9275657Z Error code E0192 needs to have at least one UI test!
2020-01-08T14:17:53.9275657Z Error code E0192 needs to have at least one UI test!
2020-01-08T14:17:53.9275724Z some tidy checks failed
2020-01-08T14:17:53.9275791Z 
2020-01-08T14:17:53.9275818Z 
2020-01-08T14:17:53.9276771Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/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"
2020-01-08T14:17:53.9276907Z 
2020-01-08T14:17:53.9276934Z 
2020-01-08T14:17:53.9282322Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2020-01-08T14:17:53.9282395Z Build completed unsuccessfully in 0:01:33
2020-01-08T14:17:53.9282395Z Build completed unsuccessfully in 0:01:33
2020-01-08T14:17:53.9341333Z == clock drift check ==
2020-01-08T14:17:53.9369980Z   local time: Wed Jan  8 14:17:53 UTC 2020
2020-01-08T14:17:54.3587814Z   network time: Wed, 08 Jan 2020 14:17:54 GMT
2020-01-08T14:17:54.3589029Z == end clock drift check ==
2020-01-08T14:17:55.1011900Z 
2020-01-08T14:17:55.1123616Z ##[error]Bash exited with code '1'.
2020-01-08T14:17:55.1154246Z ##[section]Starting: Checkout
2020-01-08T14:17:55.1156578Z ==============================================================================
2020-01-08T14:17:55.1156637Z Task         : Get sources
2020-01-08T14:17:55.1156687Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:negative-impls branch from e2a82fa to 83d1c33 Jan 8, 2020
@bors

This comment was marked as outdated.

Copy link
Contributor

bors commented Jan 8, 2020

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

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:negative-impls branch 3 times, most recently from fa21734 to a10769a Jan 8, 2020
@nikomatsakis nikomatsakis changed the title [WIP] permit negative impls for non-auto traits permit negative impls for non-auto traits Jan 8, 2020
@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 8, 2020

I've removed the WIP, I think this PR is roughly ready to land, though I think it merits an FCP and a more detailed write-up of

(a) the semantics of what is happening
(b) the motivation for this change around Pin

I can try to get to that in a bit.

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:negative-impls branch from a10769a to 4663d8f Jan 8, 2020
@varkor
varkor approved these changes Jan 8, 2020
Copy link
Member

varkor left a comment

The implementation looks good. The diagnostics around negative trait bounds in general could be improved, but that can be left as a follow up.

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Jan 8, 2020

It might also be worth putting negative impls for non-auto traits behind a separate feature flag to optin_builtin_traits, as they're really distinct features.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 8, 2020

Ah yeah, I meant to add "new feature flag" to the checklist

@Lokathor

This comment has been minimized.

Copy link
Contributor

Lokathor commented Jan 8, 2020

The "stable" version number should be 1.41 or whenever this actually gets to stable. This might sound silly to say but sometimes version strings like this have gone unnoticed so I thought i'd just throw in a reminder.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 8, 2020

Should we add !DerefMut for Rc/Arc as well (for redundancy with the orphan rules)?

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jan 8, 2020

@bors try @rust-timer queue

Let's get artifacts for a hypothetical crater run and run perf (historically blanket impls sometimes cause unexpected regressions I think)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 8, 2020

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 8, 2020

⌛️ Trying commit d199b4f with merge 573c056...

bors added a commit that referenced this pull request Jan 8, 2020
permit negative impls for non-auto traits

This is a prototype impl that extends `impl !Trait` beyond auto traits. It is not integrated with coherence or anything else, and hence only serves to prevent downstream impls (but not to allow downstream crates to rely on the absence of such impls for coherence purposes).

Fixes #66544

TODO:

- [x] need a test that you can't rely on negative impls for coherence purposes
- [x] test that negative impls cannot specialize positive ones
- [x] test that positive impls cannot specialize negative ones
- [x] extend negative impl to `Clone` in order to fully fix #66544
- [x] and maybe make `CoerceUnsized` unsafe? -- that problem is now split out into #68015
- [ ] introduce feature flag and prepare a write-up
- [x] improve diagnostics?
@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 8, 2020

@Aaron1011

Do we want to do a crater run for this?

Yes, crater run seems good. Thanks @Mark-Simulacrum for kicking that off. ❤️


@withoutboats

Should we add !DerefMut for Rc/Arc as well (for redundancy with the orphan rules)?

I like that idea, yeah. In general I would prefer for us to be using more explicit negative impls to express things we believe will never be true.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 8, 2020

☀️ Try build successful - checks-azure
Build commit: 573c056 (573c056fce8593d4c35af705123bb755d587aa98)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 8, 2020

Queued 573c056 with parent ed6468d, future comparison URL.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jan 8, 2020

@craterbot check

@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Jan 8, 2020

👌 Experiment pr-68004 created and queued.
🤖 Automatically detected try build 573c056
🔍 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.

Copy link
Collaborator

craterbot commented Jan 10, 2020

🚧 Experiment pr-68004 is now running

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

They used to be covered by `optin_builtin_traits` but negative impls
are now applicable to all traits, not just auto traits.

This also adds docs in the unstable book for the current state of auto traits.
@nikomatsakis nikomatsakis force-pushed the nikomatsakis:negative-impls branch from 1823517 to 9efb5dd Jan 10, 2020
@craterbot

This comment has been minimized.

Copy link
Collaborator

craterbot commented Jan 14, 2020

🎉 Experiment pr-68004 is completed!
📊 1 regressed and 1 fixed (81904 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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 14, 2020

The regression report looks clean -- the one regression appears to me to be spurious..?

[INFO] [stderr] error: failed to run custom build command for x86 v0.19.0
[INFO] [stderr]
[INFO] [stderr] Caused by:
[INFO] [stderr] process didn't exit successfully: /opt/rustwide/target/debug/build/x86-4914827680d6ccba/build-script-build (signal: 11, SIGSEGV: invalid memory reference)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 14, 2020

Copy link
Member

Centril left a comment

Here's the first part of my review. I've marked questions & issues I have as lang team member with (Lang) and the others not.

@@ -152,6 +152,9 @@ declare_features! (
/// Allows features specific to OIBIT (auto traits).
(active, optin_builtin_traits, "1.0.0", Some(13231), None),

/// Allow negative trait implementations.
(active, negative_impls, "1.0.0", Some(13231), None),

This comment has been minimized.

Copy link
@Centril

Centril Jan 16, 2020

Member

The introduced version should be "1.42.0".

@@ -227,4 +227,9 @@ mod impls {
*self
}
}

// Shared references can be cloned, but mutable references *cannot*!

This comment has been minimized.

Copy link
@Centril

Centril Jan 16, 2020

Member

Let's make this a doc comment?

let impl_polarity = tcx.impl_polarity(impl_def_id);
let other_polarity = tcx.impl_polarity(overlap.with_impl);
match (impl_polarity, other_polarity) {
(ty::ImplPolarity::Negative, ty::ImplPolarity::Positive) => {
report_negative_positive_conflict(
tcx,
overlap,
impl_def_id,
impl_def_id,
overlap.with_impl,
);
}
};

match tcx.span_of_impl(overlap.with_impl) {
Ok(span) => {
err.span_label(
tcx.sess.source_map().def_span(span),
"first implementation here".to_string(),
(ty::ImplPolarity::Positive, ty::ImplPolarity::Negative) => {
report_negative_positive_conflict(
tcx,
overlap,
impl_def_id,
overlap.with_impl,
impl_def_id,
);
err.span_label(
impl_span,
format!(
"conflicting implementation{}",
overlap
.self_desc
.map_or(String::new(), |ty| format!(" for `{}`", ty))
),
);
}
Err(cname) => {
let msg = match to_pretty_impl_header(tcx, overlap.with_impl) {
Some(s) => {
format!("conflicting implementation in crate `{}`:\n- {}", cname, s)
}
None => format!("conflicting implementation in crate `{}`", cname),
};
err.note(&msg);
}
}

for cause in &overlap.intercrate_ambiguity_causes {
cause.add_intercrate_ambiguity_hint(&mut err);
}

if overlap.involves_placeholder {
coherence::add_placeholder_note(&mut err);
_ => {
report_conflict(tcx, overlap, impl_def_id, used_to_be_allowed);
}
}
Comment on lines +321 to 347

This comment has been minimized.

Copy link
@Centril

Centril Jan 16, 2020

Member

Feels like this logic could be extracted out as well to keep the happy path fully clean of diagnostics logic.

match tcx.impl_polarity(impl_id) {
ty::ImplPolarity::Reservation | ty::ImplPolarity::Positive => {}
ty::ImplPolarity::Negative => {
if !impl_item_refs.is_empty() {

This comment has been minimized.

Copy link
@Centril

Centril Jan 16, 2020

Member
Suggested change
if !impl_item_refs.is_empty() {
if let [first_span, ..] = &*impl_item_refs {

(and use first_span below instead of indexing -- also the let binding feels overkill.)

This comment has been minimized.

Copy link
@KrishnaSannasi

KrishnaSannasi Jan 16, 2020

Contributor

Did you mean if let [first_span, ..]?

This comment has been minimized.

Copy link
@Centril

Centril Jan 16, 2020

Member

Yes, that... 😅 This is why writing a whole program without a compiler doesn't work... ;)

// Negative impls are not expected to have any items
match tcx.impl_polarity(impl_id) {
ty::ImplPolarity::Reservation | ty::ImplPolarity::Positive => {}
ty::ImplPolarity::Negative => {
if !impl_item_refs.is_empty() {
let first_item_span = tcx.hir().impl_item(impl_item_refs[0].id).span;
struct_span_err!(
tcx.sess,
first_item_span,
E0746,
"negative impls cannot have any items"
)
.emit();
}
return;
}
}
Comment on lines +1960 to +1976

This comment has been minimized.

Copy link
@Centril

Centril Jan 16, 2020

Member

Function is some 200 LOC now; consider extracting this to a function with -> bool (and room to improve diagnostics).

@@ -111,15 +111,14 @@ pub fn check_item_well_formed(tcx: TyCtxt<'_>, def_id: DefId) {
}
ty::ImplPolarity::Negative => {
// FIXME(#27579): what amount of WF checking do we need for neg impls?

This comment has been minimized.

Copy link
@Centril

Centril Jan 16, 2020

Member

(Lang) What is the relevance of this FIXME wrt. the future of negative impls?

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jan 17, 2020

Author Contributor

I think that if we opt for the plan that I have vaguely in mind, then we would wind up doing the same WF checking on negative impls that we do on positive impls. Effectively, Trait and !Trait become two "traits" which are not permitted to overlap.

This would be a good thing to add to the tracking issue as "to be resolved".

Negative impls cannot be default impls. A default impl supplies
default values for the items within to be used by other impls, whereas
a negative impl declares that there are no other impls. These don't
make sense to combine.

This comment has been minimized.

Copy link
@Centril

Centril Jan 16, 2020

Member

(Lang) I don't find this persuasive. This check feels like it unnecessarily complicates language semantics. It would be simpler to just allow default impl !Trait for Foo {} and then have the regular check that default? impl !Trait for Foo {} must be empty enforce that this has no effect. If you don't want to deal with this now, I'd like to see a FIXME in a test so this isn't forgotten about.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Jan 17, 2020

Author Contributor

I'm happy to register it as a "design point to be resolved later".

Let me make sure I understand your point, first. I believe what you are proposing is this. Users would be allowed to write

default impl !Trait for Foo { }

and that would be legal. However, if the default impl included any items, that would be illegal?

I guess that the general theory here is a default impl with no items in general has no real meaning? (Which seems true.)

I would find this appealing if there were some kind of code-generation or macro examples that were made easier by this by such a rule. I can't really think of any at this time, though.

That said, I don't really agree that a decision here one way or the other has any appreciable impact on the complexity of the language semantics. It seems to me that having various rules about what sorts of modifies can and cannot be combined is a pretty trivial piece of complexity.

The main benefit I see of reporting an error is that it steers users in the right direction. As the text above says, I really don't see what a default impl !Trait should even mean. Permitting it just seems to create the potential for confusion to me. However, a lint would also be a reasonable way to steer users away.

@@ -0,0 +1,3 @@
trait MyTrait {}

This comment has been minimized.

Copy link
@Centril

Centril Jan 16, 2020

Member

Let's avoid any new files directly in ui/ & let's move this into ui/traits/negative-impls/.

@@ -0,0 +1,14 @@
// run-pass

This comment has been minimized.

Copy link
@Centril

Centril Jan 16, 2020

Member

What does this test exercise? (Add a comment.)

@@ -0,0 +1,9 @@
#![feature(specialization)]

This comment has been minimized.

Copy link
@Centril

Centril Jan 16, 2020

Member

(Lang+Compiler): I didn't see any test exercising negative specializing negative:

 #![feature(specialization)]
 #![feature(negative_impls)]

trait MyTrait {}

impl<T> !MyTrait for T {}
impl !MyTrait for u32 {}

which seems fine (although not terribly exciting).

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Jan 16, 2020

Second part:

Interactions with #[marker] & #67919

Suppose that we have the following setup:

// crate A:

#[marker] trait T {}

struct S;

fn assert<X: T>() {}

// crate B:

impl !T for S {}

// crate C:

impl T for S {}

// crate D (depends on B + C):

fn main() {
    assert::<S>(); // There should be an error somewhere, but where?
}

When composing negative impls with #67919 and #[marker] traits, this seemingly reintroduces the problem we wanted the orphan rule to solve wrt. composability of crates in preventing overlap early. Ostensibly this could be solved by either a) not allowing negative impls for marker traits, or b) enforcing the orphan rule for negative impls, even for #[marker] traits.

Interactions with rust-lang/rfcs#2632

What would the interactions with impl const Trait { ... } be with negative impls?
Is impl !Trait for Struct { ... } a positive witness of impl const !Trait for Struct { ... } as well and would that extend to arbitrary effects more generally if we would introduce such (e.g. say impl no_panic !Trait for Struct { ... }).


Finally, it would be good if @arielb1 could also give the PR their blessing.

For now, I'll register a concern:

@rfcbot concern some-questions-and-stuff

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Jan 16, 2020

@guswynn

This comment has been minimized.

Copy link

guswynn commented Jan 17, 2020

A negative impl is a semver commitment never to implement the given trait for the given type...

Were the guidelines in rust-lang/rfcs#1105 ever published in official documentation? Should adding negative impl like allowed in this pr be added?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 17, 2020

@Centril

Interactions with #[marker] & #67919

Yeah, this is a good catch. I think the way to think about it is this:

  • The orphan rules express a kind of "ownership" over certain impls -- they express what sorts of impls can be added after the trait is initially created.
  • The notion of ownership is a bit complex, but it is real. This is why (as the re-re-balancing coherence RFC noted) adding blanket impls in general is a breaking change.
  • Specifically, saying that a crate C owns an impl I here means only C could add the impl I. In the first release of a trait, then clearly the crate C that adds it also owns all of the impls -- after all, nobody else can add impls until the trait exists. But after that release, ownership comes only from the orphan rules.
  • You can only add a ! impl if you have "ownership" over the positive impl.
    • That is, the only way that you can promise an impl will never be implemented, is if you are the only one that could add it in the first place.

If we permit arbitrary traits to add #[marker] implementations, then there is no ownership, and therefore they cannot have negative impls.

Interactions with rust-lang/rfcs#2632

How I understand rust-lang/rfcs#2632 is that impl const Trait for Type { .. } is a kind of "narrowing". In effect, it says, not only do I implement Trait, I do so with const fn-compatible definitions. In particular, impl const Trait for Type satisfies both Type: const Trait and Type: Trait (though the Type: const Trait syntax is not added by the RFC).

In that sense, impl !Trait for Type { .. } declares "there will never be an impl". Therefore, it implies that yes Type: const Trait and Type: Trait are both never going to happen.

Put another way, you cannot combine impl !Trait for u32 and impl const Trait for u32.

I think this would generalize to any other sort of "qualified impls" that we wish to add.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 17, 2020

@guswynn

Were the guidelines in rust-lang/rfcs#1105 ever published in official documentation? Should adding negative impl like allowed in this pr be added?

Good question. I don't know. I think the answer is 'no', people were just directed to the RFC, but I think we should create such docs, and adjust them to include negative impls.

I think that the Rust reference might actually be a suitable place for those docs.

@guswynn

This comment has been minimized.

Copy link

guswynn commented Jan 17, 2020

Good question. I don't know. I think the answer is 'no', people were just directed to the RFC, but I think we should create such docs, and adjust them to include negative impls.

I think that the Rust reference might actually be a suitable place for those docs.

@nikomatsakis Sorry to hijack this pr for a learning moment, do you know where I could get started on trying to help contribute to the rust reference?

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.