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

Use hir::Path instead of ast::Path in HIR Attribute's. #56480

Closed
wants to merge 1 commit into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Dec 3, 2018

The goal here was to avoid using ast::Path in the HIR and having serialization logic for ast::{Ty,Expr,...} (inside generic arguments), despite them not existing for attribute paths.
We can't remove Nonterminals "as easily", but we bug! for those already in some places.

This seemed a trivial change at first, but now I'm not so sure, I've had to make a lot of code work on either AST or HIR Attributes, using generics (over the Path parameter).

Maybe we should just have path: Vec<Ident> in Attribute, instead of {ast,hir}::Path?
EDIT: opened #56492 for that variant.

r? @petrochenkov cc @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2018
@rust-highfive

This comment has been minimized.

@petrochenkov
Copy link
Contributor

We can't remove Nonterminals "as easily", but we bug! for those already in some places.

That's what I immediately thought - even if AST paths are removed, there still can be pieces of AST in tokens.

Is it possible to bug!() on generic arguments in AST paths as well?
I think that would be the best solution for the short term problem of unblocking #56511.
(If some generic arguments still reach HIR as a consequences of error recovery, they can be filtered away during lowering.)

@petrochenkov
Copy link
Contributor

Actually, looking at #56511 I no longer understand why pieces of AST in HIR is an issue.
#56511 removes encodable/decodable imple from HIR, but AST is still serializable, so what's the problem?

@petrochenkov
Copy link
Contributor

Regarding attributes in HIR, I the long term goal is to have some semantic representation for attributes in HIR (hir::Attribute) based on name resolution results, and not just tokens.
This means no hir::MetaItem, hir::MetaItemKind etc, because no token parsing happens on HIR attributes, all the parsing and validation is done during AST -> HIR lowering.
hir::Attributes would probably still contain paths (hir::Path), but mostly for diagnostics.

In that sense this PR is closer to the long term goal than #56492.

@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 Dec 5, 2018
@bors
Copy link
Contributor

bors commented Dec 6, 2018

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

@XAMPPRocky
Copy link
Member

Triage; @eddyb Hello, have you been able to get back to this PR?

@TimNN
Copy link
Contributor

TimNN commented Jan 22, 2019

Ping from triage @eddyb: What is the status of this PR?

@Dylan-DPC-zz
Copy link

ping from triage @eddyb Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again!

@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. labels Jan 28, 2019
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants