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

Split {ast,hir,...}::Static(Mutability) into Static and StaticMut. #60110

Closed
wants to merge 1 commit into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Apr 19, 2019

Static is currently the only variant of Def that describes an Item and contains more than the DefId of that item (i.e. it contains the mutability).
This lack of uniformity is making some refactors/cleanups a bit harder, or more annoying.

There are primarily two alternatives, AFAICT:

  1. make StaticMut its own kind of item/def (this PR)
  2. have everything that cares about the mutability, query it (like is_const_fn)
    • specifically, when working with a DefId and not {ast,hir}::ItemKind::Static

I was hoping 1. would be simpler than it turned out to be, but I did the work anyway, so I'm presenting this PR for people to judge which approach they prefer.
(e.g. all the changes to Static | StaticMut, in patterns, would be unnecessary for 2.)

cc @petrochenkov @oli-obk @nikomatsakis @michaelwoerister

@rust-highfive
Copy link
Contributor

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 19, 2019
@petrochenkov petrochenkov self-assigned this Apr 19, 2019
@Centril
Copy link
Contributor

Centril commented Apr 19, 2019

make StaticMut its own kind of item/def (this PR)

This seems like a step backwards wrt. #54912 and extending it to arbitrary patterns, e.g. static (A, mut B) = ...;. It also seems to me that this invites semantic divergence between statics and static muts.

@cramertj
Copy link
Member

r? @oli-obk

@petrochenkov petrochenkov self-assigned this Apr 19, 2019
@petrochenkov
Copy link
Contributor

This is impractical, IMO.
Mutable and immutable statics behave identically in most cases, so it's more convenient to have a single ast::Static, hir::Static and Def::Static and query mutability when necessary.

See the next comment regarding data in Def.

@petrochenkov
Copy link
Contributor

petrochenkov commented Apr 19, 2019

How I see Def for items having DefId.

Def consists of two components:

  • DefId, the primary component.
  • A cache of commonly used data associated with that DefId. This data is not independent and can be obtained from DefId alone (from HIR map or metadata), but it's more convenient to have it readily available.

The most commonly used cached data is the "Def kind" / "Def discriminant".
An example of other data that is commonly used and cached is the constructor kind in Def::Ctor items.
(Constructors have more similarity between them than differences, so it's less convenient / more bug-prone to have separate Def variants for them.)

Mutability in Def::Static is not commonly used, so it can be removed from the cache, like #60124 does.
I'm not sure how having more data in the cache prevents refactorings though.
(Note that this PR keeps the mutability in the cache by having two different variants for statics despite it not being commonly used.)

@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 Apr 19, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Apr 20, 2019

I definitely prefer #60124. In fact my first reaction to this PR was "why isn't it using the is_static query" only to realize that it's not a query.

@eddyb
Copy link
Member Author

eddyb commented Apr 20, 2019

FWIW, I'm already doing the refactor in question, and Def::Static doesn't hurt as much as I was worried it would. I like #60124, and I'm closing this PR. Thanks!

@eddyb eddyb closed this Apr 20, 2019
@eddyb eddyb deleted the separate-static-mut branch April 20, 2019 14:25
bors added a commit that referenced this pull request Apr 21, 2019
Remove mutability from `Def::Static`

Querify `TyCtxt::is_static`.
Use `Mutability` instead of bool in foreign statics in AST/HIR.

cc #60110
r? @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants