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

include description of bindings in the reference #654

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nikomatsakis
Copy link
Contributor

This describes the behavior implemented in rust-lang/rust#63376.

cc @Centril

Copy link
Contributor

@Havvy Havvy left a comment

Choose a reason for hiding this comment

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

I can't verify correctness, so these are all editorial requests.

src/lifetime-elision.md Outdated Show resolved Hide resolved
>
```

**Note:** As of this writing, the rules for associated type bindings
Copy link
Contributor

Choose a reason for hiding this comment

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

"As of this writing" is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, this phrase suggests a temporary condition that is expected to change. It doesn't feel redundant to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**Note:** As of this writing, the rules for associated type bindings
**Note:** As of 2019-08-16, the rules for associated type bindings

src/lifetime-elision.md Outdated Show resolved Hide resolved
src/lifetime-elision.md Outdated Show resolved Hide resolved
src/lifetime-elision.md Outdated Show resolved Hide resolved
* If there is more than one bound from the containing type then an explicit
bound must be specified

If the trait object is used as a binding for an associated type (e.g.,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the "e.g." parenthetical is useful here. But if we keep it, it should say "for example". It'd be nice if "trait object" and "associated type" were links. Or link to the full example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, perhaps the e.g. isn't the most elegant way to express it, but I think that showing the syntax will go along way towards helping people understand what the other words actually mean.

I can definitely make links.

Separate but relevant: In general, I tend to think we should deprecate the term trait objects and move towards "dyn types" or something like that, but that'd be a shift obviously. I do think though that leveraging the keyword in our terminology makes a lot of sense.

@@ -71,9 +71,17 @@ rules.
If the trait object is used as a type argument of a generic type then the
containing type is first used to try to infer a bound.

* If there is a unique bound from the containing type then that is the default
* If there is a unique bound from the containing type then that is the default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If there is a unique bound from the containing type then that is the default.
* If there is a unique lifetime bound from the containing type then that is the default.

@@ -71,9 +71,17 @@ rules.
If the trait object is used as a type argument of a generic type then the
containing type is first used to try to infer a bound.

* If there is a unique bound from the containing type then that is the default
* If there is a unique bound from the containing type then that is the default.
* If there is more than one bound from the containing type then an explicit
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If there is more than one bound from the containing type then an explicit
* If there is more than one lifetime bound from the containing type then an explicit


If the trait object is used as a binding for an associated type (e.g.,
`Item = dyn Trait`) then the containing trait is first used to try to
infer a bound, in an analogous way to type arguments:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
infer a bound, in an analogous way to type arguments:
infer a lifetime bound, in an analogous way to type arguments:

`Item = dyn Trait`) then the containing trait is first used to try to
infer a bound, in an analogous way to type arguments:

* If there is a unique bound from the containing type then that is the default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If there is a unique bound from the containing type then that is the default.
* If there is a unique lifetime bound from the containing type then that is the default.

infer a bound, in an analogous way to type arguments:

* If there is a unique bound from the containing type then that is the default.
* If there is more than one bound from the containing type then an explicit
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If there is more than one bound from the containing type then an explicit
* If there is more than one lifetime bound from the containing type then an explicit

dyn Bar<
'x
Item1 = dyn Baz, // defaults to `dyn Baz + 'static`
Item2 = dyn Baz, // defaults to 'dyn Baz + `x`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great and more complicated (which is why it is great) example to include as a test in the PR!

@ehuss
Copy link
Contributor

ehuss commented Apr 24, 2020

@nikomatsakis I was looking to merge this with the minor editorial changes, but I see that this example doesn't compile (aside from the syntax issues). I assume this is because of rust-lang/rust#63618.

IIUC, the rules for defaults for associated types are not implemented at all? If that is correct, then I personally would prefer to document what is implemented, instead of what is intended in this case. (Following the same kind of logic that we don't document unstable features, future intentions, unimplemented RFCs, etc.)

If there are cases that are implemented, then I think it would be better to use those as examples. If they are not implemented at all, then I would prefer to remove the associated type rules for now and change the example to make it clear that it fails to infer.

We can very easily add the rules back once they are implemented.

@nikomatsakis
Copy link
Contributor Author

@ehuss oof, I totally forgot about this PR.

@Havvy
Copy link
Contributor

Havvy commented Feb 14, 2021

What's the status on this?

@traviscross
Copy link
Contributor

Per the comment above here...

@rustbot author
@rustbot labels +S-blocked

@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. S-blocked Status: Marked as blocked on further work. labels Nov 12, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 3, 2024

☔ The latest upstream changes (possibly bf115a4) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked on further work. S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants