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

Make lang-items private #301

Closed
doctorn opened this issue Jun 4, 2020 · 7 comments
Closed

Make lang-items private #301

doctorn opened this issue Jun 4, 2020 · 7 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@doctorn
Copy link

doctorn commented Jun 4, 2020

TL;DR

tcx.lang_items().my_lang_item().unwrap() is an anti-pattern which triggers ICEs in #![no_core]. This MCP proposes to refactor the current implementation of lang-items to prevent it.

Links and Details

In #![no_core], we cannot assume that any lang-items are present. At present, due to

the compiler should be free from these kind of ICEs. However, tcx.lang_items().my_lang_item().unwrap() is an anti-pattern that we want to avoid reintroducing in the future. Following some discussion the idea (credit to @ecstatic-morse) is to introduce an opaque option representing the presence (or lack thereof) of lang-items:

pub enum LangItemRecord {
    DefId(DefId),
    Missing(LangItem),
}

To avoid having to explicitly match on this enum everywhere where we normally do something like

if Some(def_id) == tcx.lang_items().my_lang_item() {
    // ...
}

methods would be implemented for LangItemRecord to allow for comparison with DefIds (silently returning false if the item is missing).

By including the Missing variant, I am hoping that the missing and items field of the LanguageItems struct can be unified and we can pre-allocate an array of LangItemRecords with each initially assumed Missing. This is likely a longer term proposal though and the immediate priority is to prevent unwraps.

The rustc-dev-guide is also sadly lacking with respect to descriptions of the various kinds of lang-items and how they're used so I'll be working on supporting documentation as part of this work.

Mentors or Reviewers

@oli-obk
@ecstatic-morse
(Assuming both of you are still interested 😅)

@doctorn doctorn added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Jun 4, 2020
@rustbot rustbot added the to-announce Announce this issue on triage meeting label Jun 4, 2020
@nikomatsakis
Copy link
Contributor

@rustbot second -- this seems like a good idea to me, though I think @ecstatic-morse or @oli-obk should review.

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Jun 4, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jun 4, 2020

Jup, I'll gladly do any reviews on this

@nagisa
Copy link
Member

nagisa commented Jun 4, 2020

While I'm supportive of the idea, I'd like to point out #![no_core] IIRC is unstable and is effectively a libcore-exclusive thing anyway, so any ICEs arising as part of using #![no_core] are either people using an unstable feature "wrong" or a bug in the rust-lang/rust repo.

(Consequentially, its fine if we adjust the language item design further. I remember @eddyb wanting to change something about how intrinsics/language items worked).

@bjorn3
Copy link
Member

bjorn3 commented Jun 4, 2020

so any ICEs arising as part of using #![no_core] are either people using an unstable feature "wrong"

Indeed. For cg_clif I have a crate called mini_core that I use in place of libcore for some tests as it compiles much faster and when I started libcore didn't compile at all. Sometimes when implementing new features in cg_clif I have to expand mini_core. It has happened several times that I declared a lang item wrong and the compiler gave no clue about what the problem was.

@nikomatsakis
Copy link
Contributor

Yes to all of that. (In my view, no-core is "perma-unstable": that is, we never intend to support people providing their own variants of core.)

@bjorn3
Copy link
Member

bjorn3 commented Jun 10, 2020

In my view, no-core is "perma-unstable": that is, we never intend to support people providing their own variants of core.

I know my use case is pretty exotic. I have seen at least one other user: lrs, which is an alternative for libcore that among other things doesn't have any functions that panic.

@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Jun 10, 2020
@nikomatsakis
Copy link
Contributor

@bjorn3 to be clear, I also have a "mini core" that I use for debugging purposes. I don't mean that we shouldn't have the capability, but I do mean that we shouldn't expose it on stable, and I think that we should feel free to make changes to libcore impl. details and lang item definitions liberally.

@spastorino spastorino added major-change-accepted A major change proposal that was accepted and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

7 participants