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.

Sign up for GitHub

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

Jump to bottom

Refactor global paths #38232

Merged
merged 2 commits into from
Dec 23, 2016
Merged

Refactor global paths #38232

merged 2 commits into from
Dec 23, 2016

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Dec 8, 2016 •
edited
Loading

This PR removes the field global: bool from ast::Path and hir::Path, instead representing a global path ::foo::bar as {{root}}::foo::bar, where {{root}} is a virtual keyword keywords::CrateRoot.

Also, fixes #38016.

r? @nrc

@jseyfried
Copy link
Contributor Author

jseyfried commented Dec 8, 2016 •
edited
Loading

This is groundwork for hygiene 2.0, specifically hygienic global paths between crates.

Since we will need the CrateRoot ident's ctxt to hygienically resolve semantically global paths, use foo; must parse into use {{root}}::foo; (use foo; and use ::foo; now parse into the same AST).

Alternatively, we could instead support hygienic global paths between crates by adding a field ctxt: SyntaxContext to today's Path that we would mark in the Marker fold.

@jseyfried jseyfried force-pushed the refactor_global_paths branch 2 times, most recently from 156a9b9 to 74b8d67 Compare December 8, 2016 09:24
@jseyfried
Copy link
Contributor Author

cc @eddyb @petrochenkov

@jseyfried jseyfried force-pushed the refactor_global_paths branch 3 times, most recently from bcd5a06 to 47eac3f Compare December 8, 2016 23:21
@petrochenkov
Copy link
Contributor

I wanted to review this PR, but forgot, sorry.

Is the intent to eliminate $crate and just use ::path in macros (2.0)?
I looked through the diff and this refactoring doesn't seem to be especially useful for that goal, neither it removes significant amount of code.
Does this affect results of -Z hir-stats noticeably?
Maybe Option<Marker> could be used instead (None for lexical paths and Some(..) for global)?

@jseyfried jseyfried closed this Dec 12, 2016
@jseyfried jseyfried reopened this Dec 12, 2016
@jseyfried
Copy link
Contributor Author

jseyfried commented Dec 12, 2016 •
edited
Loading

@petrochenkov

Is the intent to eliminate $crate and just use ::path in macros (2.0)?

Yeah, global paths should be hygienic by default for macros 2.0, so we won't need $crate.

this refactoring doesn't seem to be especially useful for that goal

This refactoring will allow us to use nearly the same logic to resolve {{root}} hygienically as we currently use to resolve $crate, which I think is nice aesthetically. That being said, it would also be simple to resolve hygienically if Path instead had a field SyntaxContext or Option<SyntaxContext>.

Does this affect results of -Z hir-stats noticeably?

I haven't measured it, but I don't expect this to have a significant effect on hir stats. I could instead leave hir paths unchanged and just refactor ast paths -- IIRC refactoring hir paths only caused fallout in the lowerer and pretty printer.

ast::Path is part of the largest variant in a couple of ast enums though, and this refactoring did slightly improve memory usage and performance during parsing and expansion. The perf improvement should be more pronounced after the "Optimize ast::PathSegment" commit from #38171 lands -- I'll measure more thoroughly then.

@jseyfried jseyfried force-pushed the refactor_global_paths branch from 47eac3f to e8fe46a Compare December 15, 2016 05:41
@bors
Copy link
Contributor

bors commented Dec 15, 2016

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

@jseyfried jseyfried force-pushed the refactor_global_paths branch 3 times, most recently from ae2f1a8 to 550b4e1 Compare December 15, 2016 08:55
nrc
nrc approved these changes Dec 20, 2016
View reviewed changes
Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

r=me with the minor refactoring

src/libsyntax/print/pprust.rs
first = false
} else {
let mut segments = path.segments[..path.segments.len()-depth].iter();
if defaults_to_global && path.segments[0].identifier.name == keywords::CrateRoot.name() {
Copy link
Member

Choose a reason for hiding this comment

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

I think I would like an is_global method on paths, rather than having checks like this (path.segments[0].identifier.name == keywords::CrateRoot.name()) scattered around the code. It will also make it easier for tools using the AST.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point -- will do.

src/libsyntax/print/pprust.rs
try!(self.print_ident(segment.identifier));

try!(self.print_path_parameters(&segment.parameters, colons_before_params));
if segment.identifier.name != keywords::CrateRoot.name() {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps also for segments

@jseyfried jseyfried force-pushed the refactor_global_paths branch 2 times, most recently from e9b84d0 to 0bd6eab Compare December 20, 2016 21:28
@bors
Copy link
Contributor

bors commented Dec 21, 2016

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

@jseyfried jseyfried force-pushed the refactor_global_paths branch from 0bd6eab to be2a531 Compare December 21, 2016 06:32
@jseyfried
Copy link
Contributor Author

@nrc I added a commit to fix #38016 by pretty-printing $crate::path as ::path (as discussed on IRC).

@jseyfried jseyfried mentioned this pull request Dec 21, 2016
Closed
@nrc
Copy link
Member

nrc commented Dec 21, 2016

r = me, but you have test failures now

@jseyfried jseyfried force-pushed the refactor_global_paths branch from be2a531 to 8a1acb2 Compare December 22, 2016 06:14
@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Dec 22, 2016

📌 Commit 8a1acb2 has been approved by nrc

@bors
Copy link
Contributor

bors commented Dec 22, 2016

⌛ Testing commit 8a1acb2 with merge b86cb75...

@bors
Copy link
Contributor

bors commented Dec 22, 2016

💔 Test failed - auto-mac-32-opt

@jseyfried
Copy link
Contributor Author

@bors retry

@bors
Copy link
Contributor

bors commented Dec 23, 2016

⌛ Testing commit 8a1acb2 with merge 82611a0...

bors added a commit that referenced this pull request Dec 23, 2016
@bors
Auto merge of #38232 - jseyfried:refactor_global_paths, r=nrc
82611a0 
Refactor global paths

This PR removes the field `global: bool` from `ast::Path` and `hir::Path`, instead representing a global path `::foo::bar` as `{{root}}::foo::bar`, where `{{root}}` is a virtual keyword `keywords::CrateRoot`.

Also, fixes #38016.

r? @nrc
@bors
Copy link
Contributor

bors commented Dec 23, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: nrc
Pushing 82611a0 to master...

@bors bors merged commit 8a1acb2 into rust-lang:master Dec 23, 2016
This was referenced Dec 23, 2016
Merged
Merged
@jseyfried jseyfried deleted the refactor_global_paths branch January 5, 2017 01:56
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jun 7, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nrc nrc nrc approved these changes

Assignees

@nrc nrc

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Eliminate $crate in --pretty=expanded
4 participants
@jseyfried @petrochenkov @bors @nrc