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

Separate foreign items in HIR #37713

Closed
nikomatsakis opened this issue Nov 11, 2016 · 26 comments
Closed

Separate foreign items in HIR #37713

nikomatsakis opened this issue Nov 11, 2016 · 26 comments
Labels
A-HIR Area: The high-level intermediate representation (HIR) A-incr-comp Area: Incremental compilation C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 11, 2016

PR #37660 separates impl items from impls in the HIR for improved interaction with incremental compilation. It probably makes sense to separate foreign items in this way too -- not so much for improved incremental compilation precision, as because it makes the layout more analogous.

@nikomatsakis nikomatsakis added A-incr-comp Area: Incremental compilation E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Nov 11, 2016
@nikomatsakis
Copy link
Contributor Author

Marking this as E-mentor. I'll give a short write-up here; happy to provide a more detailed one too upon request!

The work that needs to be done is basically to model foreign items on impl items -- we would get a hir::ForeignItemRef like hir::ImplItemRef, a separate map in the Krate to store them, and modify the lowering routines analogously. Finally, there are a few places (marked with FIXMEs attached to this issue number) where we could convert to using a ItemLikeVisitor if foreign items were considered "item like".

@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Nov 11, 2016
@eddyb
Copy link
Member

eddyb commented Nov 11, 2016

Something I've wanted to do for a while is just remove the pointless "foreign mod" nonsense in HIR.
We just need to stick the attributes from the extern {...} somewhere at the crate level or something.

@aochagavia
Copy link
Contributor

For someone who has never worked on anything hir-related... Could you give examples of Rust code that compiles down to:

  • An ImplItem?
  • A ForeignItem?

@petrochenkov
Copy link
Contributor

@aochagavia

impl Trait for Type {
    fn f() {} // <-- ImplItem
}
impl Type {
    fn g() {} // <-- ImplItem
}
extern "C" {
    fn printf(); // <-- ForeignItem
}

@aochagavia
Copy link
Contributor

So an ImplItem is any item that appears within the block of an impl? And a ForeignItem is any item that appears inside an extern block? What about extern "C" fn foo() { }?

@petrochenkov
Copy link
Contributor

So an ImplItem is any item that appears within the block of an impl? And a ForeignItem is any item that appears inside an extern block?

Yes.

What about extern "C" fn foo() { }?

It's just a Fn Item using non-default ABI.

@aochagavia
Copy link
Contributor

Thanks!

Now some more questions, about what @eddyb said:

  1. Could you give examples of Rust code that are represented as a ForeignMod? Is my interpretation below correct?
extern "C" { // <-- ForeignMod?
    fn printf(); // <-- ForeignItem
}
  1. What needs to be done in order to get rid of the "pointless foreign mod nonsense"? Would you like ForeignMod to be removed? What should happen with the data then?

@petrochenkov
Copy link
Contributor

Is my interpretation below correct?

Yes.

What needs to be done in order to get rid of the "pointless foreign mod nonsense"?

Suppose we have a foreign module like this:

#[link(name = "qwerty")]
extern "C" {
    fn f();
}

, then foreign items (f) need to be transplanted from the foreign module into the normal non-foreign parent module, ABI ("C") moved from the foreign module to foreign items, and attributes like #[link(name = "qwerty")] moved... somewhere.

@aochagavia
Copy link
Contributor

Does that mean that ForeignMod wouldn't be needed anymore? Or are there other types of ForeignMod?

@petrochenkov
Copy link
Contributor

petrochenkov commented Dec 12, 2016

Yeah, the whole point of this exercise is to get rid of ForeignMod (in HIR).

@eddyb
Copy link
Member

eddyb commented Dec 12, 2016

One question is whether we might want to have a bunch of imports that share some common information - AFAIK all the attributes on an extern "ABI" {...} don't affect the imports inside but rather the crate.

cc @retep998

@retep998
Copy link
Member

#[link] affects each individual import in terms of whether to apply dllimport. #[cfg] obviously affects whether the imports even exist. I can't think of any other attributes that can be applied to an extern "ABI" {...}.

@eddyb
Copy link
Member

eddyb commented Dec 12, 2016

@retep998 Is the dllimport thing something we could have as a flag on each "foreign item"?
#[cfg] is already gone by the time of the HIR lowering so that's not an issue.

Ugh I guess lint scoping is a thing. I just don't want ForeignMod and ForeignItem to exist 😞.

@retep998
Copy link
Member

@eddyb Each foreign item needs to know which library it comes from so that when rustc is informed of what type of library it is (static and static-nobundle vs dylib) it can emit dllimport properly. You can ask @vadimcn for more info on that.

@vadimcn
Copy link
Contributor

vadimcn commented Dec 13, 2016

These associations are extracted pretty early on and stored in CStore (indexed by DefId), so I don't think there's any dependency on hir::ItemForeignMod for this feature.

@eddyb
Copy link
Member

eddyb commented Dec 13, 2016

@retep998 @vadimcn I'm not sure it's feasible to lift "foreign items" to the surrounding scope, even if just because of the lint scoping issue, i.e. #[allow(x)] extern {...} applies to everything inside.

If we had a way of cheaply duplicating such attributes... maybe, but still. It's annoying because they're almost regular items, and having them separate from all the other items is complexity I'd like to get rid of.

@cramertj
Copy link
Member

@aochagavia Are you working on this? If not, I'd like to try it.

@eddyb
Copy link
Member

eddyb commented Jan 25, 2017

@nikomatsakis Did we even come to an consensus on this? It'd be nice not to have "foreign items".

@aochagavia
Copy link
Contributor

@cramertj Go ahead! I never figured out what exactly needed to happen, so I didn't even start working on it.

@cramertj
Copy link
Member

@aochagavia If I understand @eddyb's comment correctly, it doesn't sound like there's been a decision as to what should be done.

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Jan 27, 2017

@eddyb to be perfectly honest, I've not been following the discussion that closely. I just read over it. I agree with your intuition that it would be nice to lift them, but there seem to be some practical hurdles in the way:

  • First, that there is still a concept of a "foreign item" that these items are embedded within; I believe that is relevant to linking both on Windows (dllimport) and Mac OS/X (frameworks), but I'm no expert
  • Second, that we would want to respect lint scoping.

It seems to me that my original plan -- to keep the ForeignMod in the HIR but make foreign items into "item-like" things -- would have much of the same benefit. If you use visit_all_item_likes_in_hir, then you can ignore the existence of ForeignMod, and if you care, it's still there.

Ultimately it seems like ForeignMod is a concept we will have to represent, just like impls and traits, whether we do it by having an item in the HIR or by having something "on the side".

@eddyb
Copy link
Member

eddyb commented Jan 27, 2017

Can we move them to Item and effectively keep ForeignMod around as a weird sort of module?
And/or point to the NodeId of the ForeignMod from each item lifted from it, without actual parenting.

@nikomatsakis
Copy link
Contributor Author

@eddyb

Reordering slightly:

Can we [...] point to the NodeId of the ForeignMod from each item lifted from it, without actual parenting.

I considered this, but it seems not obviously good. e.g., it means that the HIR will not reflect the DefPath hierarchy, and lints will have to work around it.

Can we move them to Item and effectively keep ForeignMod around as a weird sort of module?

This is sort of interesting-- since name resolution is already done, I guess we don't care too much. I'm still having trouble seeing the value here, I have to admit, over just making them item-like. Perhaps there is lots of code that gets simplified? I guess that compared to impl-item and trait-item, foreign-item seems like small potatoes.

@nikomatsakis
Copy link
Contributor Author

(But maybe we can think of an approach that works for all of those things?)

@tamird
Copy link
Contributor

tamird commented Apr 8, 2018

Seems like this is not fully baked enough to be E-mentor.

@cramertj cramertj removed the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Apr 8, 2018
@jonas-schievink jonas-schievink added the A-HIR Area: The high-level intermediate representation (HIR) label Apr 18, 2020
@cjgillot
Copy link
Contributor

Solved by #79318.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-HIR Area: The high-level intermediate representation (HIR) A-incr-comp Area: Incremental compilation C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants