Skip to content

Use def span for non-ascii ident feature gate error#46803

Merged
bors merged 1 commit intorust-lang:masterfrom
estebank:non-ascii-def-span
Dec 27, 2017
Merged

Use def span for non-ascii ident feature gate error#46803
bors merged 1 commit intorust-lang:masterfrom
estebank:non-ascii-def-span

Conversation

@estebank
Copy link
Copy Markdown
Contributor

@estebank estebank commented Dec 18, 2017

No description provided.

@rust-highfive
Copy link
Copy Markdown
Contributor

rust-highfive commented Dec 18, 2017

r? @arielb1

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

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 18, 2017
@arielb1
Copy link
Copy Markdown
Contributor

arielb1 commented Dec 18, 2017

I'm worried this might call def_span on random-ish AST nodes (whatever is passed to visit_name), with possibly random results. What are the sort of nodes that are supposed to be valid input to def_span?

@estebank
Copy link
Copy Markdown
Contributor Author

estebank commented Dec 18, 2017

@arielb1 def_span is an outright hack that takes everything in the original span until the next occurrence of the char {, or the full span if it reaches newline before then. The case where this could be a problem would be if a user tried something like type My{Weird}Type;, but that would get caught by the parser.

bjorn3
bjorn3 reviewed Dec 19, 2017
View reviewed changes
Comment thread src/test/ui/feature-gate-non_ascii_idents.stderr
--> $DIR/feature-gate-non_ascii_idents.rs:34:5
|
34 | / Bäz { //~ ERROR non-ascii idents
35 | | qüx: isize //~ ERROR non-ascii idents
Copy link
Copy Markdown
Member

@bjorn3 bjorn3 Dec 19, 2017

Choose a reason for hiding this comment

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

Why doesn't this line give an error?

Copy link
Copy Markdown
Contributor Author

@estebank estebank Dec 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Good question. It wasn't already, would have to check.
@bjorn3 that error is being emitted further down, it's just hidden in the diff because it didn't change in this PR :)

@arielb1
Copy link
Copy Markdown
Contributor

arielb1 commented Dec 25, 2017

r? @petrochenkov

@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented Dec 26, 2017

@bors r+

@bors
Copy link
Copy Markdown
Collaborator

bors commented Dec 26, 2017

📌 Commit 502d570 has been approved by petrochenkov

@bors
Copy link
Copy Markdown
Collaborator

bors commented Dec 27, 2017

⌛ Testing commit 502d570 with merge e8098c5...

bors added a commit that referenced this pull request Dec 27, 2017
@bors
Auto merge of #46803 - estebank:non-ascii-def-span, r=petrochenkov
e8098c5 
Use def span for non-ascii ident feature gate error
@bors
Copy link
Copy Markdown
Collaborator

bors commented Dec 27, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing e8098c5 to master...

@bors bors merged commit 502d570 into rust-lang:master Dec 27, 2017
@bors bors mentioned this pull request Dec 27, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@petrochenkov petrochenkov

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@estebank @rust-highfive @arielb1 @petrochenkov @bors @bjorn3 @kennytm