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 #72240

Open
doctorn opened this issue May 15, 2020 · 16 comments
Open

Make lang items private #72240

doctorn opened this issue May 15, 2020 · 16 comments
Labels
A-lang-item Area: Language items C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@doctorn
Copy link
Contributor

doctorn commented May 15, 2020

#72170 and #72216 put some effort in to try and prevent some potential ICEs that can occur in #![no_core] due to how we currently reference lang items.

@oli-obk pointed out:

Oh, that's a very good idea. I wonder if we can make lang_items private at some point and just have sensible wrappers like require_lang_item.

I'm opening this issue to start a discussion about how we could take this forward.

@doctorn
Copy link
Contributor Author

doctorn commented May 15, 2020

Here are my thoughts at the moment:

  • The main thing we want to avoid is calling tcx.lang_items().<item>().unwrap() as in #![no_core] this is a hard panic
  • Using tcx.require_lang_item(<Item>, span) trivially resolves this
  • Ideally we would just remove all uses of tcx.lang_items().<item>() and remove lang_items for the type context's public API
  • There's a problem in that the following is a pattern used throughout the compiler in diagnostic generation:
if Some(def_id) == tcx.lang_items().<item>() {
    // some smart diagnostic
}
  • This can't be swapped for def_id == tcx.require_lang_item(<Item>, span) because we don't want to trigger a fatal error if the lang item isn't found, we just don't want to emit the diagnostic

My feeling at the moment is that something along the lines of tcx.is_lang_item(<Item>, def_id) is the tidiest way to solve this, but obviously this is going to touch a lot of code so I thought it was worth a discussion.

@oli-obk
Copy link
Contributor

oli-obk commented May 15, 2020

If we want to make it clear that this should only be used for diagnostics, we could name the function check_lang_item_for_diagnostic or sth similar. It's a mouth full, but at least this means noone will mis-use it. Another alternative is to make it a method on DiagnosticBuilder that takes a closure and only invokes the closure if the DefId is the specified lang item. This way it definitely can't be mis-used and is on the right API.

@lcnr
Copy link
Contributor

lcnr commented May 15, 2020

The easiest way might be to add tcx.get_lang_item(LangItem) -> Option<DefId> with a doc comment recommending require_lang_item in case the lang item is required.

@doctorn
Copy link
Contributor Author

doctorn commented May 15, 2020

I definitely agree that we need to make sure it's not misused, although refactoring this sort of thing



to use a modification like that to the DiagnosticBuilder API might be more trouble than it's worth...

@lcnr lcnr added A-lang-item Area: Language items C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 15, 2020
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented May 15, 2020

I think we should use a custom return type, not Option, for the result of tcx.lang_items().<item>(). It would have a require method that takes a span and a TyCtxt and works the same way as require_lang_item, plus an exists method that returns a bool. This makes it much harder to misuse the API and you can still match on it to extract the def_id.

@doctorn
Copy link
Contributor Author

doctorn commented May 15, 2020

The easiest way might be to add tcx.get_lang_item(LangItem) -> Option<DefId> with a doc comment recommending require_lang_item in case the lang item is required.

I think we'd have to be careful with this as I imagine this is the sort of thing that will slip through almost all code reviews

@doctorn
Copy link
Contributor Author

doctorn commented May 15, 2020

I think we should use a custom return type, not Option, for the result of tcx.lang_items().<item(>). It would have a require method that takes a span and works the same way as require_lang_item and an exists method that converts to a bool. This makes it much harder to misuse the API.

I really like this idea - although to make sure lang_items stays private, we'd probably want to move it up a level. Something like tcx.query_lang_item(<Item>) that returns the new opaque option.

We could just bolt a is_def_id(def_id) method on the API for the opaque option then as well to deal with the diagnostic pattern.

@oli-obk
Copy link
Contributor

oli-obk commented May 15, 2020

to use a modification like that to the DiagnosticBuilder API might be more trouble than it's worth...

Oh wow... that !is_unsize definitely kills that idea, agreed.

I really like this idea - although to make sure lang_items stays private, we'd probably want to move it up a level. Something like tcx.query_lang_item(<Item>) that returns the new opaque option.

I think the suggested idea is to make the LangItems type's functions all private, so we don't need to make lang_items private, as we can just add the is and expect functions to it as public functions and expose no further API

@doctorn
Copy link
Contributor Author

doctorn commented May 15, 2020

Ahh okay I misunderstood... So the change would be something like:

  • introducing an opaque option for lang items
  • updating the current lang_items API to return the new wrapper in place of the old options
  • adding is and expect methods for the new wrapper
  • reimplementing the current methods like tcx.require_lang_item to use the new API
  • updating uses of the Some(def_id) == ... pattern to use is

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented May 15, 2020

I think the suggested idea is to make the LangItems type's functions all private, so we don't need to make lang_items private,

That's what I had in mind, yes. I like avoiding the need for imports (e.g., FutureTraitLangItem) when possible.

@doctorn
Copy link
Contributor Author

doctorn commented May 15, 2020

Okay yeah - I really like that, I'll have a go at implementing it then

@doctorn
Copy link
Contributor Author

doctorn commented May 15, 2020

@rustbot claim

@nikomatsakis
Copy link
Contributor

(Side note: discussing on the issue seems fine, but this seems like an MCP-worthy thing.)

@doctorn
Copy link
Contributor Author

doctorn commented May 16, 2020

@nikomatsakis I spoke to @oli-obk about whether an MCP was necessary before I opened this issue but I don’t think either of us appreciated the amount of code this was going to effect and therefore didn’t think it was necessary. I don’t really know anything about the MCP process as I’ve been out of the loop for a while but I’m more than happy to learn about it and take this down that route.

@Alexendoo
Copy link
Member

Triage: Hi, are you still working on this issue @doctorn?

@Alexendoo
Copy link
Member

@rustbot release-assignment

@rustbot rustbot removed their assignment Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lang-item Area: Language items C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants