Move def id collection and extern crate handling to before AST->HIR lowering #33089

nrc · 2016-04-19T04:59:09Z

eddyb · 2016-04-19T05:04:33Z

So, uhm, what about the elephant in the room? The AST changes during expansion, do you still plan on using IDs even with resolve-while-expanding?
If you have the plan, I'm quite curious, but atm I'm not seeing how this isn't a step backwards.

nrc · 2016-04-19T05:14:54Z

@eddyb - this stuff happens after expansion, but before lowering, not before expansion. This would allow us to use def ids in the HIR and name resolution as part of lowering. It isn't intended to directly allow def ids for use in name resolution in expansion. If we do want to use def ids for that, then I imagine we continue to collect def ids during expansion, in the same way that we do for lowering here. I think that works because def ids are monotonic - once we are able to assign a def id to a node, it will never change due to further expansion.

eddyb · 2016-04-19T05:20:29Z

@nrc I think this is relying way too much on there being IDs embedded in nodes to begin with.

However, using name resolution during lowering (or rather, turning resolve into the lowering pass) is great, so I'm happy in that regard, just worried about tight coupling between DefId and NodeId.

One step at a time, I guess.

jseyfried · 2016-04-19T07:43:04Z

@eddyb

relying way too much on there being IDs embedded in nodes

What is the motivation for removing the IDs in AST nodes?

We will need to compute scopes and resolve import and macro paths on the AST during expansion. Without embedded IDs, I believe we would have to come up with a node identification system that would not be invalidated by expansion. This is certainly doable, but it would add considerable complexity.

eddyb · 2016-04-19T12:27:19Z

@jseyfried The only reason the whole "assign IDs and build a map" thing works is because the AST (now HIR) is frozen at that point.
How do you plan to access nodes based on IDs in a constantly folding AST without fundamentally changing the representation?

bors · 2016-04-19T15:06:51Z

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

jseyfried · 2016-04-19T20:25:27Z

@eddyb

How do you plan to access nodes based on IDs in a constantly folding AST

By relying on @nrc's monotonicity property. Since macro definitions and invocations are not monotonic, I plan on using a separate "id namespace" for them or assigning them normally and then reassigning ids after expansion to avoid unused ids (this would still be simpler than a custom identification system, imo).

More specifically, I plan on keeping track of the next normal id and the next macro invocation id during expansion. After each expansion, we would assign all unassigned nodes appropriately and collect new def ids.

eddyb · 2016-04-19T21:24:14Z

@jseyfried So the "catch" is that you can waste IDs. What do you think about storing the entire AST in monotonic vectors instead of using pointers?
As long as the AST isn't inspected and modified routinely after parsing/manual creation (with the exception of expansion), the AST could be created with an identity, since we seem to want that.
Having smaller IDs instead of pointers should help with the memory usage (which is a tad bit ridiculous, compared to the actual information it's expressing).

An alternative which still allows random access would be a flattened stream of "commands" which form the AST, with positions in that stream as the pre-HIR/Ty identification.
Think ADT+sexprs with no pointers, just a byte buffer. It's a long shot, don't get me wrong, just want to know if that kind of experimentation will even be possible.

jseyfried · 2016-04-19T21:33:11Z

So the "catch" is that you can waste IDs.

I don't think we would waste any ids if we use a separate "id namespace" for macro invocations and if we always remove unconfigured items before assigning new ids.

jseyfried · 2016-04-19T21:41:10Z

What do you think about storing the entire AST in monotonic vectors instead of using pointers?

I definitely like this idea, although it would be a massive plugin-[breaking-change], of course.

a flattened stream of "commands" which form the AST, with positions in that stream as the pre-HIR/Ty identification.

I think this would be possible, but I'm not sure it would be worth the complexity. Specifically, I think it would be a challenge to remain backwards compatible with the current macro system, in which the expansion order is significant.

eddyb · 2016-04-19T21:49:04Z

@jseyfried I thought the plan was to ditch expansion order? But I agree, none of what I said is trivial.
Thanks for taking the time to explain it, I see how this can unfold now.
Definitely not as bad as some of the non-O(1) identity solutions.

The changes in this PR LGTM, although I can't wait to get rid of the AST/HIR embedded in crate metadata.

…es for the HIR map.

And move extern crate reading earlier in the driver

So that we can work with inlined HIR from metadata.

jseyfried · 2016-04-19T22:19:13Z

@eddyb

I thought the plan was to ditch expansion order?

It is, but we still need to support, for example:

macro_rules! foo { () => {} }
foo! {} // resolves to ::foo
#[macro_use] mod bar {
    macro_rules! foo { () => {} }
}
foo! {} // resolves to ::bar::foo

Thinking about this some more, I don't think it would be too hard to support this with a "stream of commands" based AST since we should be able to perform all "old-style" resolutions during the first round of expansion if we expand "depth first" as we currently do (i.e. immediately re-expand macro-generated AST instead of waiting for the next expansion round).

nikomatsakis · 2016-04-20T20:10:45Z

@jseyfried you kind of lost me a bit -- why do we need to have two namespaces to cope with macro expansion?

jseyfried · 2016-04-20T21:04:58Z

@nikomatsakis
We need two namespaces not for correctness but to avoid wasting node ids on macro invocations, which never end up in the HIR and so only need ids during expansion.

Immediately after expanding a macro, I'm planning to remove unconfigured items and assign node ids. With the exception of macro invocations, all the assigned nodes will eventually become part of the HIR, so their ids won't be wasted. If we assigned ids for macro invocations independently of other nodes (i.e. we use a separate NodeIdAssigner for macro invocations), then we could identify an AST node with a type like:

enum AstNodeId {
    MacroInvocation(NodeId),
    OtherNode(NodeId),
}

We could also start the macro invocation id assigner at 0x80000000 and then use raw node ids to identify AST nodes (provided that there will not be more than 2^31 nodes, of course).

jseyfried · 2016-04-21T02:24:24Z

src/librustc/hir/lowering.rs

+                                            false,
+                                            result_ident,
+                                            match_expr,
+                                            None);


nit: this doesn't need to be changed (unless you prefer the new formatting -- I usually prefer to save vertical space)

jseyfried · 2016-04-21T03:30:38Z

Reviewed, LGTM modulo optional comments.

I wasn't very familiar with of def id collection before reviewing this, so I might not be the best qualified to approve -- r? @eddyb

eddyb · 2016-04-21T10:43:03Z

@bors r+

bors · 2016-04-21T10:43:04Z

📌 Commit 0be3c8c has been approved by eddyb

bors · 2016-04-21T15:37:06Z

⌛ Testing commit 0be3c8c with merge 34886d6...

bors · 2016-04-21T17:52:14Z

💔 Test failed - auto-win-msvc-64-opt-mir

alexcrichton · 2016-04-21T18:02:09Z

@bors: retry

On Thu, Apr 21, 2016 at 10:52 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-msvc-64-opt-mir
http://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt-mir/builds/386

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#33089 (comment)

bors · 2016-04-22T10:41:30Z

⌛ Testing commit 0be3c8c with merge a264f5b...

@jseyfried

Move def id collection and extern crate handling to before AST->HIR lowering r? @jseyfried, @eddyb, or @nikomatsakis

bors · 2016-04-22T14:00:58Z

nrc added 9 commits April 20, 2016 10:13

Trivial refactoring

c99b73a

Split up NodeCollector so that defs are collected separately from nod…

f61b404

…es for the HIR map.

Move DefCollector to its own module.

d6bcc04

Separate def collection and hir map making even further

6af7aca

def_collector and crate reader operate on AST instead of HIR

84c3f89

And move extern crate reading earlier in the driver

refactoring

0c37d4b

HIR visitor for DefCollector

744be0b

So that we can work with inlined HIR from metadata.

debugging, misc fixes

1d5a29c

rebasing

0be3c8c

nrc force-pushed the hir-name-res branch from 68ba46c to 0be3c8c Compare April 19, 2016 22:16

jseyfried reviewed Apr 21, 2016
View reviewed changes

bors added a commit that referenced this pull request Apr 22, 2016

Auto merge of #33089 - nrc:hir-name-res, r=eddyb

a264f5b

Move def id collection and extern crate handling to before AST->HIR lowering r? @jseyfried, @eddyb, or @nikomatsakis

bors merged commit 0be3c8c into rust-lang:master Apr 22, 2016

This was referenced Apr 22, 2016

Refactor pretty printing to use the compiler API #33119

Merged

Thread tighter span for closures around #33125

Merged

Overhaul borrowck error messages and compiler error formatting generally #32756

Merged

jseyfried mentioned this pull request Jun 13, 2016

Allow MultiItemModifiers to expand into zero or many items #34253

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move def id collection and extern crate handling to before AST->HIR lowering #33089

Move def id collection and extern crate handling to before AST->HIR lowering #33089

nrc commented Apr 19, 2016

eddyb commented Apr 19, 2016

nrc commented Apr 19, 2016

eddyb commented Apr 19, 2016

jseyfried commented Apr 19, 2016

eddyb commented Apr 19, 2016 •

edited

Loading

bors commented Apr 19, 2016

jseyfried commented Apr 19, 2016 •

edited

Loading

eddyb commented Apr 19, 2016

jseyfried commented Apr 19, 2016 •

edited

Loading

jseyfried commented Apr 19, 2016 •

edited

Loading

eddyb commented Apr 19, 2016 •

edited

Loading

jseyfried commented Apr 19, 2016 •

edited

Loading

nikomatsakis commented Apr 20, 2016

jseyfried commented Apr 20, 2016 •

edited

Loading

jseyfried Apr 21, 2016

jseyfried commented Apr 21, 2016 •

edited

Loading

eddyb commented Apr 21, 2016

bors commented Apr 21, 2016

bors commented Apr 21, 2016

bors commented Apr 21, 2016

alexcrichton commented Apr 21, 2016

bors commented Apr 22, 2016

bors commented Apr 22, 2016

Move def id collection and extern crate handling to before AST->HIR lowering #33089

Move def id collection and extern crate handling to before AST->HIR lowering #33089

Conversation

nrc commented Apr 19, 2016

eddyb commented Apr 19, 2016

nrc commented Apr 19, 2016

eddyb commented Apr 19, 2016

jseyfried commented Apr 19, 2016

eddyb commented Apr 19, 2016 • edited Loading

bors commented Apr 19, 2016

jseyfried commented Apr 19, 2016 • edited Loading

eddyb commented Apr 19, 2016

jseyfried commented Apr 19, 2016 • edited Loading

jseyfried commented Apr 19, 2016 • edited Loading

eddyb commented Apr 19, 2016 • edited Loading

jseyfried commented Apr 19, 2016 • edited Loading

nikomatsakis commented Apr 20, 2016

jseyfried commented Apr 20, 2016 • edited Loading

jseyfried Apr 21, 2016

Choose a reason for hiding this comment

jseyfried commented Apr 21, 2016 • edited Loading

eddyb commented Apr 21, 2016

bors commented Apr 21, 2016

bors commented Apr 21, 2016

bors commented Apr 21, 2016

alexcrichton commented Apr 21, 2016

bors commented Apr 22, 2016

bors commented Apr 22, 2016

eddyb commented Apr 19, 2016 •

edited

Loading

jseyfried commented Apr 19, 2016 •

edited

Loading

jseyfried commented Apr 19, 2016 •

edited

Loading

jseyfried commented Apr 19, 2016 •

edited

Loading

eddyb commented Apr 19, 2016 •

edited

Loading

jseyfried commented Apr 19, 2016 •

edited

Loading

jseyfried commented Apr 20, 2016 •

edited

Loading

jseyfried commented Apr 21, 2016 •

edited

Loading