After libsyntax, idents should be replaced by Names #6993

jbclements · 2013-06-07T16:25:48Z

This is a FIXME issue; right now, an ident contains information used by expansion. After expansion, this is collapsed down to a single Name (an index into the interner). However, the downstream stuff in librustc refers to "ast::ident" everywhere. It would save space and time to just pass around one int rather than two, by replacing uses of "ident" with "Name". I don't know if I'll have time to do this. (Planning for my untimely demi^H^Hparture.)

jbclements · 2013-06-26T21:52:12Z

More details on the "right way" to fix this.

resolve.rs, generally speaking, builds tables and then resolves identifiers by looking things up in them. Sometimes, this lookup should respect renamings (e.g. lexical bindings). Other times, it shouldn't (e.g., module names).

during expansion, identifiers gain "syntax contexts"; at the end of expansion, each ident has a uint telling what name it started with, and what syntax context it occurred in. There's an operation called mtwt_resolve (that is, the "resolve" operation specified in the MTWT paper) that maps this to a new name.

Ideally, then, this mtwt_resolve would be called when building the tables of lexical bindings, and not called otherwise. This would probably be done by having most of the tables just map Names (that is, uint indexes into the interner) to whatever-it-is-they-map-to.

Currently, however, these tables all map idents to whatever-it-is. I should fix this, but frankly, I'm just scrambling to get through, so I'm taking the short-cut of using the existing data structure rather than updating them.

pnkfelix · 2013-06-27T07:17:30Z

For those following along, "the MTWT paper" presumably refers to "Macros That Work Together"

bblum · 2013-08-19T22:47:14Z

AIUI this is just an internal cleanup and possible performance issue. Nominating for no milestone; let's see if the triage committee agrees.

pnkfelix · 2013-08-20T11:01:11Z

(For what it's worth, I don't think "nominating for no-milestone" explicitly via the I-nominated tag for an Issue that already has no milestone is an appropriate use of the nomination protocoll. More specifically, IMO the triage committee should only be looking at tickets that need their milestone setting changed and where there exists some champion for that change. @graydon any thoughts on process here?)

graydon · 2013-08-20T13:31:12Z

Yeah, nomination means you think the milestone assignment should change. Removing label.

update AST so that ExprBreak and ExprCont expressions contain names, not idents. Fixes #9047 and makes progress on #6993. Simplifies the compiler very slightly, should make it (infinitesimally) faster.

emberian · 2013-12-30T22:00:32Z

Triage: still a bunch of work to do on this.

cc rust-lang#6993

flaper87 · 2014-02-11T09:56:40Z

I started working on this, I'll keep doing so!

ahmedcharles · 2014-05-30T19:52:30Z

Is anyone still working on this?

jbclements · 2014-05-31T13:20:01Z

I'm going to be working on macros again for a month, starting in a few weeks. I guess that qualifies as a "yes".

bgamari · 2014-07-06T13:56:29Z

Any updates here?

jbclements · 2014-07-06T17:49:17Z

I'm working on hygiene, but not this specific issue. Still looks like a nice fix.

This does a large chunk of #6993. It is based on top of #17654.

emberian · 2015-03-16T07:42:06Z

Looking through a git grep Ident librustc*, it seems this is pretty close to being done.

…ch of rust-lang#6993.

@pcwalton

...of #6993. cc @pcwalton

petrochenkov · 2015-09-17T22:34:23Z

I'm working on this.
So far I see two subtasks to work on:

There's a bunch of random code everywhere using Idents unconsciously, it can be rewritten to work with Names unconditionally and without much doubt.
Some parts of resolve (all code calling mtwt::resolve) legitimately need SyntaxContext information from Idents to resolve hygienic names. So, HIR has to keep at least some information about macro expansion despite the general desire to get rid of it. The good news is that many structures in HIR (at least TraitRef, Generics, TyPath, PatStruct, ExprStruct, PatEnum, PatQPath, but not ExprPath) can drop syntax context information at cost of splitting hir::Path into hir::NamePath and hir::IdentPath (with Name and Ident segments respectively) and some duplication in resolve in code performing path resolution. This duplication can be considered a drawback, so this subtask is not as unambiguous as the previous one. I'd be glad to hear some opinions on this matter.

I'll likely submit two separate PRs to cover these two tasks in order.

petrochenkov · 2015-09-17T22:38:25Z

HIR visitors are somewhere between these two tasks, but I tend to convert them to use Names. The logic is that syntax context information is needed for very narrow task in resolve and is not supposed to be used by general visitors.

arielb1 · 2015-09-17T22:40:11Z

@petrochenkov

We may want to move resolve to the AST.

petrochenkov · 2015-09-17T22:45:14Z

@arielb1
At least the first my subtask doesn't prevent this move.
(Considering the second one, I'll try to do it anyway just to see what will come of it)

petrochenkov · 2015-09-17T22:52:52Z

cc #28139 @nrc

jbclements · 2015-09-17T23:04:51Z

Somewhat off-topic, but I'd love to come up to Mountain View and talk about Macros 2.0 some time.

nrc · 2015-09-22T00:19:18Z

@petrochenkov I'd be strongly against splitting Paths in the manner you suggest, I'd much rather just carry around a little extra redundant information than have to deal with that.

@jbclements we haven't made much progress with macros 2.0 tbh, I'll be sure to ping you once we get stuck in.

@nrc

Part of #6993 This patch replaces `Ident`s with `Name`s in data structures of HIR and updates the dependent crates to compile and pass `make check`. Some HIR structures still use `Ident`s, namely `PathSegment`, `PatIdent`, `ExprWhile`, `ExprLoop`, `ExprBreak` and `ExprAgain`, they need them for resolve (but `PathSegment` is special, see #6993 (comment)). r? @nrc

petrochenkov · 2015-09-23T10:19:20Z

@nrc

I'd be strongly against splitting Paths in the manner you suggest, I'd much rather just carry around a little extra redundant information than have to deal with that.

I've just tried to do it and it indeed turned out to be too much trouble for the little expected gain. I'll omit this part. The second PR will just remove some remaining random Idents.

@nrc

This PR removes random remaining `Ident`s outside of libsyntax and performs general cleanup In particular, interfaces of `Name` and `Ident` are tidied up, `Name`s and `Ident`s being small `Copy` aggregates are always passed to functions by value, and `Ident`s are never used as keys in maps, because `Ident` comparisons are tricky. Although this PR closes #6993 there's still work related to it: - `Name` can be made `NonZero` to compress numerous `Option<Name>`s and `Option<Ident>`s but it requires const unsafe functions. - Implementation of `PartialEq` on `Ident` should be eliminated and replaced with explicit hygienic, non-hygienic or member-wise comparisons. - Finally, large parts of AST can potentially be converted to `Name`s in the same way as HIR to clearly separate identifiers used in hygienic and non-hygienic contexts. r? @nrc

Improve `expl_impl_clone_on_copy` fixes: rust-lang#1254 changelog: Check to see if the generic constraints are the same as if using derive for `expl_impl_clone_on_copy`

…giraffate Update docs for `expl_impl_clone_on_copy` The known issue was fixed in rust-lang#6993. I missed updating the docs then, so it's happening now. changelog: None

jbclements mentioned this issue Sep 10, 2013

more idents changed to names #9103

Closed

flaper87 added a commit to flaper87/rust that referenced this issue Feb 11, 2014

Replace Ident with Name in for loops

bef6fbe

cc rust-lang#6993

flaper87 mentioned this issue Feb 11, 2014

Replace Ident with Name in for loops #12179

Closed

flaper87 self-assigned this Feb 11, 2014

flaper87 mentioned this issue Feb 14, 2014

break and continue are not hygienic #12262

Closed

jbclements unassigned flaper87 Jun 16, 2014

gereeter mentioned this issue Oct 1, 2014

Part of #6993: Move a bunch of uses of Ident to Name #17683

Merged

gereeter pushed a commit to gereeter/rust that referenced this issue Oct 23, 2014

Part of rust-lang#6993. Moved a bunch of uses of Ident to Name

2343e9d

bors added a commit that referenced this issue Oct 23, 2014

auto merge of #17683 : gereeter/rust/ident-to-name, r=pcwalton

56d544f

This does a large chunk of #6993. It is based on top of #17654.

pnkfelix mentioned this issue Feb 10, 2015

Make FRU respect privacy of all struct fields, mentioned or unmentioned. #22144

Merged

gereeter mentioned this issue Mar 24, 2015

In librustc*, convert many uses of ast::Ident to ast::Name, fixing much ... #23655

Merged

gereeter added a commit to gereeter/rust that referenced this issue Apr 3, 2015

In librustc*, convert many uses of ast::Ident to ast::Name, fixing mu…

3cbc345

…ch of rust-lang#6993.

bors added a commit that referenced this issue Apr 6, 2015

Auto merge of #23655 - gereeter:rustc-remove-ident, r=sanxiyn

aab8669

...of #6993. cc @pcwalton

petrochenkov mentioned this issue Sep 13, 2015

Lints on AST, remove ExprParen #28349

Merged

petrochenkov mentioned this issue Sep 20, 2015

Replace Idents with Names in HIR (Part 1) #28535

Merged

This was referenced Sep 24, 2015

Replace Idents with Names outside of libsyntax + Idents/Names cleanup (Part 2) #28642

Merged

libsyntax: Consider using Names instead of most Idents in AST datastructures #28659

Closed

bors closed this as completed in #28642 Sep 26, 2015

oli-obk mentioned this issue Nov 29, 2018

Suggestion Span for non_upper_case_globals could be more precise #48103

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

After libsyntax, idents should be replaced by Names #6993

After libsyntax, idents should be replaced by Names #6993

jbclements commented Jun 7, 2013

jbclements commented Jun 26, 2013

pnkfelix commented Jun 27, 2013

bblum commented Aug 19, 2013

pnkfelix commented Aug 20, 2013

graydon commented Aug 20, 2013

emberian commented Dec 30, 2013

flaper87 commented Feb 11, 2014

ahmedcharles commented May 30, 2014

jbclements commented May 31, 2014

bgamari commented Jul 6, 2014

jbclements commented Jul 6, 2014

emberian commented Mar 16, 2015

petrochenkov commented Sep 17, 2015

petrochenkov commented Sep 17, 2015

arielb1 commented Sep 17, 2015

petrochenkov commented Sep 17, 2015

petrochenkov commented Sep 17, 2015

jbclements commented Sep 17, 2015

nrc commented Sep 22, 2015

petrochenkov commented Sep 23, 2015

After libsyntax, idents should be replaced by Names #6993

After libsyntax, idents should be replaced by Names #6993

Comments

jbclements commented Jun 7, 2013

jbclements commented Jun 26, 2013

pnkfelix commented Jun 27, 2013

bblum commented Aug 19, 2013

pnkfelix commented Aug 20, 2013

graydon commented Aug 20, 2013

emberian commented Dec 30, 2013

flaper87 commented Feb 11, 2014

ahmedcharles commented May 30, 2014

jbclements commented May 31, 2014

bgamari commented Jul 6, 2014

jbclements commented Jul 6, 2014

emberian commented Mar 16, 2015

petrochenkov commented Sep 17, 2015

petrochenkov commented Sep 17, 2015

arielb1 commented Sep 17, 2015

petrochenkov commented Sep 17, 2015

petrochenkov commented Sep 17, 2015

jbclements commented Sep 17, 2015

nrc commented Sep 22, 2015

petrochenkov commented Sep 23, 2015