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

Add Captures marker trait #56047

Closed
wants to merge 1 commit into from
Closed

Conversation

alexreg
Copy link
Contributor

@alexreg alexreg commented Nov 18, 2018

Implements #56046.

@alexreg
Copy link
Contributor Author

alexreg commented Nov 18, 2018

@rust-lang/libs I've done very little library work before, mainly working with the compiler. How do I add a feature gate for the library? Are feature-gate tests done in the same way as for compiler features?

@rust-highfive
Copy link
Collaborator

r? @TimNN

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 18, 2018
@rust-highfive
Copy link
Collaborator

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:057ed3f3:start=1542558230365434616,finish=1542558231480502105,duration=1115067489
$ 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

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)

@SimonSapin
Copy link
Contributor

I believe #[unstable] as already used here is enough, regarding feature-gating.

However, please add to both the doc-comment and the description of the tracking issue: what this API does, why it exists, an example of how it is used.

You link to #34511 (comment) to justify the existence of this feature, but that is a comment in the middle of the tracking issue for impl Trait type, which has very many comments, so it’s hard to understand what the context is.

Is this new API expected to be associated with language changes in how impl Trait type interact with borrow checking? While the tracking issue is still open, some of the functionality of impl Trait type is already available on Stable. Are these language-level changes still possible now or would they be breaking changes?

@alexreg
Copy link
Contributor Author

alexreg commented Nov 19, 2018

@SimonSapin I don't believe there are any plans to change the language semantics for RPIT lifetimes, but I could be wrong. This solution is ambivalent to such potential changes, as I see it.

Regarding the tracking issue and doc comment: happy to add this once I can get a minimal working example. The doc comment already explains what it does, though the "why" and example do indeed need to be added. Will have a think about all this. I just wanted to get this issue formally out there in the open for now.

@nikomatsakis @aturon Thoughts and opinions on any of the above would be welcome, given your comments about this on the aforementioned tracking issue.

@nikomatsakis
Copy link
Contributor

My opinion is that we should fix the language. I'm not entirely sure how to justify that yet though. It seems like you should be able to "hide" lifetimes that appear in the result but which are not relevant to the interface, just as you can do with a dyn Trait. (Hiding a lifetime 'a would require that you are capturing some other lifetime 'b where 'a: 'b, just as hiding lifetimes in a dyn type requires a trait object bound 'b.)

@TimNN
Copy link
Contributor

TimNN commented Nov 27, 2018

r? @aturon (or @nikomatsakis or @rust-lang/libs)

@TimNN TimNN assigned aturon and unassigned TimNN Nov 27, 2018
@alexreg
Copy link
Contributor Author

alexreg commented Nov 27, 2018

@TimNN We've decide to take a separate approach to this. @eddyb left @nikomatsakis some suggestions on this on Discord, which he will be reviewing shortly, I think. :-)

@TimNN TimNN added A-allocators Area: Custom and system allocators and removed A-allocators Area: Custom and system allocators labels Dec 4, 2018
@bors
Copy link
Contributor

bors commented Dec 8, 2018

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

@Dylan-DPC-zz
Copy link

ping from triage @alexreg @eddyb any updates on this?

@alexreg
Copy link
Contributor Author

alexreg commented Jan 7, 2019

@Dylan-DPC @eddyb had some ideas for @nikomatsakis about this (and making it a language feature). I think it's waiting for the two of them to talk before something can be done, I'm afraid.

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 8, 2019
@alexreg alexreg closed this May 12, 2019
@apiraino apiraino removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants