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

Stabilize Ident::new_raw #59002

Closed
wants to merge 1 commit into from

Conversation

adnanademovic
Copy link

Tracking issue: #54723

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 7, 2019
@alexcrichton alexcrichton added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 8, 2019
@alexcrichton
Copy link
Member

There's some discussion about stabilization in the tracking issue and it looks like one of the main questions is about the name of the API (Ident::new_raw, Ident::raw, etc). @petrochenkov notes that raw identifiers are now stabilized which was one of the main reasons (IIRC) for not stabilizing this from before.

In light of all that I'd personally be in favor of the current new_raw name and would like to propose stabilization! I've tagged T-libs for the API aspect and T-lang as well because they were tagged on the tracking issue. If T-lang y'all don't feel you need to sign off, I'll cancel and re-issue FCP!

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 8, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 8, 2019
@kennytm kennytm added the relnotes Marks issues that should be documented in the release notes of the next release. label Mar 8, 2019
@Centril
Copy link
Contributor

Centril commented Mar 8, 2019

(Including T-Lang on this was proper as this exposes language features programmatically...)

I agree that the name is fine; I would also be alright with Ident::new -- either way works and it's not a big deal either way.

I do however think that the documentation on the function is a bit underwhelming; In particular, the semantics of new_raw is unclear wrt. panics (e.g. for r#self... -- cc @petrochenkov). On the subject of clear semantics, I'd also like to be pointed towards the set of tests (run-pass, run-fail, etc.) that provide checks on the semantics... To that end:

@rfcbot concern tests-and-clearer-docs

@Centril Centril added this to the 1.35 milestone Mar 8, 2019
@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 Mar 18, 2019
@Centril Centril modified the milestones: 1.35, 1.36 Apr 12, 2019
@Centril Centril modified the milestones: 1.36, 1.37 May 31, 2019
@Centril
Copy link
Contributor

Centril commented Jun 13, 2019

ping @adnanademovic @alexcrichton re. my request for clearer docs and a list of tests ^---

@Centril Centril modified the milestones: 1.37, 1.38 Jul 2, 2019
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.38, 1.39 Aug 11, 2019
@cramertj
Copy link
Member

Ping again for docs, tests so we can stabilize.

@Centril Centril modified the milestones: 1.39, 1.40 Sep 26, 2019
@Mark-Simulacrum Mark-Simulacrum 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 24, 2019
@@ -812,7 +812,7 @@ impl Ident {
}

/// Same as `Ident::new`, but creates a raw identifier (`r#ident`).
Copy link
Contributor

@petrochenkov petrochenkov Oct 26, 2019

Choose a reason for hiding this comment

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

One more line:

/// In addition to general identifier validity requirements, raw identifiers cannot be created for keywords usable in path segments (`self`, `super` and others).

Copy link
Contributor

Choose a reason for hiding this comment

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

That leaves me somewhat confused; what does "general identifier validity requirements" entail here? Surely Ident::new_raw has fewer restrictions than Ident::new_raw but your wording suggests it has more -- in particular, when will Ident::new_raw panic?

Copy link
Contributor

Choose a reason for hiding this comment

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

"general identifier validity requirements" -> "general identifier validity requirements inherited from Ident::new".
We can point to the reference or something that describes what is an identifier.

Surely Ident::new_raw has fewer restrictions than Ident::new_raw but your wording suggests it has more

It does have more restrictions.

in particular, when will Ident::new_raw panic?

"raw identifiers cannot be created for" -> "this function will panic on attempts to create"

Copy link
Contributor

Choose a reason for hiding this comment

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

It does have more restrictions.

Ah! $ident accepts keywords as well whereas it does not accept r#self -- that's pretty counter-intuitive. I think amending the documentation of fn new would be good as well to note the fact that keywords are allowed.

@petrochenkov
Copy link
Contributor

Tests that need to be added:

  • Proc-macro-generated struct r#fn; parses and creates a usable struct named r#fn.
  • Proc-macro-generated r#struct S; doesn't parse.
  • Proc-macro-generated struct r#S; parses and creates a usable struct named S.

Existing tests:

  • Cannot create a raw identifier from a path segment keyword - ui/proc-macro/invalid-punct-ident-3.rs.
  • Non proc-macro-generated raw identifiers are pretty thoroughly tested, search r#\w in src/test/**/*.rs for examples.

@petrochenkov petrochenkov 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 Oct 26, 2019
@Centril Centril modified the milestones: 1.40, 1.41 Nov 7, 2019
@Dylan-DPC-zz
Copy link

@adnanademovic any updates on this?

@JohnCSimon
Copy link
Member

Ping from triage:
@adnanademovic Can you please provide an update on the status of this PR
cc: @petrochenkov @Centril
Thank you!

@JohnCSimon
Copy link
Member

Pinging from triage:
@adnanademovic Can you please provide an update on the status of this PR?

Thank you.

@adnanademovic
Copy link
Author

Hi, sorry for the lack of response. I was extremely busy. If the 3 tests are all it's needed, I'll try to do it in the next few days. I just have to go learn how to actually compile tests!

@joshtriplett
Copy link
Member

(Checking @aturon's box, as he's no longer on the lang team.)

@Centril Centril modified the milestones: 1.41, 1.42 Dec 20, 2019
@hdhoang
Copy link
Contributor

hdhoang commented Jan 9, 2020

ping from triage @adnanademovic, any updates on this?

@Centril Centril modified the milestones: 1.42, 1.43 Jan 31, 2020
@Dylan-DPC-zz
Copy link

Closing this since it has been inactive for a long time. Thanks for the effort :)

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 1, 2020
@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. relnotes Marks issues that should be documented in the release notes of the next release. labels Feb 1, 2020
@Centril Centril removed this from the 1.43 milestone Mar 10, 2020
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Aug 3, 2020
Tracking issue: rust-lang#54723

This is a continuation of PR rust-lang#59002
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 3, 2020
…r=petrochenkov

Stabilize Ident::new_raw

Tracking issue: rust-lang#54723

This is a continuation of PR rust-lang#59002
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 4, 2020
…r=petrochenkov

Stabilize Ident::new_raw

Tracking issue: rust-lang#54723

This is a continuation of PR rust-lang#59002
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet