rename ImplItem_::*ImplItem to ImplItem_::*

[oli-obk]

[breaking change]

I'm not sure if those renames are ok. TokenType::Tt* to TokenType::* was obvious, but for all those Item-enums it's less obvious to me what the right way forward is due to the underscore.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

cc @rust-lang/compiler, thoughts about naming here?

[eddyb]

I was pondering doing a different kind of rename, Item -> ItemNode and Item_ -> Item, to allow using, e.g. Item::Trait instead of ItemTrait (same for other AST nodes, not just Item).
But the "node" type is still used a lot so it's not a great solution.

[nrc]

I approve - I think we should make more use of namespaced enums like this. I also agree with eddyb that it would be nice not to have _ in enum names (the proper solution, I think, is more sophisticated concrete data types, part of the efficient inheritance work).

[eddyb]

The common enum fields part of ADT hierarchies would indeed solve this long-standing problem quite elegantly, but I'm not sure what the timeline for those is.

[nikomatsakis]

I've been wanting to do something like this, but I am not a big fan of the _ names, I would want to adjust that naming scheme first.

[alexcrichton]

r? @eddyb (moving to random compilers team member)

[nikomatsakis]

So, I'm going to take a stand and say that I do not want to type ast::Item_::Foo. I think the usual convention in this situation is to rename Item_ to ItemKind (and probably rename the node field to kind), which is sort of the opposite of what @eddyb suggested. It has the downside that one must write ast::ItemKind::Foo, which is long, but the upside that the "main type" is ast::Item and that you access item.kind to get a value of type ItemKind (whereas in @eddyb's proposal you access item.node to get a value of type Item). I could go either way on which renaming we do, but I think if we're going to change to use namespace'd variants for Item and friends, we should rename at the same time.

[oli-obk]

addressed the comments and also did the renaming for ast::ImplItem_::*

[bors]

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

[oli-obk]

rebased

[nikomatsakis]

@bors r+

ok, since conversation is sort of dead, let's just do this thing. We can always adjust the naming scheme later. Thanks @oli-obk. (Merge conflicts here we come!)

[bors]

📌 Commit d09220d has been approved by nikomatsakis

[bors]

⌛ Testing commit d09220d with merge b31cc64...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt