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

Rewrite for loop desugaring to use language items #1457

Closed
wants to merge 1 commit into from
Closed

Rewrite for loop desugaring to use language items #1457

wants to merge 1 commit into from

Conversation

apasel422
Copy link
Contributor

@durka
Copy link
Contributor

durka commented Jan 10, 2016

This requires a good way for HIR lowering to refer to lang items (currently they are collected after lowering). cc rust-lang/rust#30809 @eddyb

# Drawbacks
[drawbacks]: #drawbacks

Possible backward compatibility concerns with existing `#![no_core]` code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel there might/are more backwards incompat vectors here. I’m pretty sure you can replace libstd and libcore in at least one way: by replacing libcore or libstd in their well known locations with another libraries. rustc --extern core=*.rlib is also a viable way of “replacing” a libcore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nagisa That counts as much as forking Rust does: we don't guarantee anything, nor could we.

@eddyb
Copy link
Member

eddyb commented Jan 10, 2016

Since lang items are an implementation detail of rustc, I do not see the need for a RFC.

I personally prefer using lang items instead of hardcoding paths, and would like to see a proper discussion on rust-lang/rust#30809.

cc @rust-lang/compiler

@nrc nrc added the T-compiler Relevant to the compiler team, which will review and decide on the RFC. label Jan 10, 2016
@nrc
Copy link
Member

nrc commented Jan 10, 2016

@eddyb RFCs for implementation details of the compiler are allowed and encouraged! It's what we have a T-compiler label for.

I also agree we should do this, in fact I filed rust-lang/rust#20541 about the same thing a year ago (back when the desugarings were in libsyntax). It's still not clear to me how that should work though, I'd like to see some ideas here or elsewhere for how this could be implemented

@eddyb
Copy link
Member

eddyb commented Jan 10, 2016

@nrc Right, I want the discussion, but it felt that this RFC was dealing with language semantics and not implementation details (I see this change as a bugfix). Hopefully the tagging solves that.

To expand on what was suggested in rust-lang/rust#30809:
We could have an enum in librustc_front of all the lang items it's going to use, and a map:

enum FrontLangItem {
    IntoIteratorIntoIter,
    IteratorNext,
    Range,
    OptionSome,
    OptionNone
    ...
}

fn lower(krate: ast::Crate) -> (hir::Crate, NodeMap<FrontLangItem>) {...}

Then resolve can take that map, look up HIR nodes in there, and if they are found, ignore the actual path, and convert the FrontLangItem into a librustc lang item DefId to be stored in the def_map.

@nrc
Copy link
Member

nrc commented Jan 10, 2016

Hmm, a lazy approach to lang items, that sounds promising. The approach I had considered before was that we could just move the lang item machinery earlier in the compiler and allow resolving lang items to fully qualified paths as well as actual references. Not sure if that will actually work.

@nikomatsakis
Copy link
Contributor

I agree with @eddyb that this probably doesn't rise to the level of complexity that necessitates an RFC, but it does seem like a reasonably good idea to me, presuming we can rearrange the lang item business and make that work out.

@nikomatsakis
Copy link
Contributor

That said, I should note that I don't feel especially motivated to support #[no_core]. I consider core and the compiler pretty tied together and don't expect to ever stabilize or really support #[no_core]. If there are use cases for which core is really unsuitable, I'd like to know more about them and see if we can fix that (perhaps through some sort of target-specific subsetting or what have you).

@nikomatsakis
Copy link
Contributor

Hear ye, hear ye. This RFC is now entering final comment period.

The conclusion in the meeting was that while this may or may not have truly needed an RFC, there is one now and, since it generated some mild discussion and controversy, we might as well accept it like any other.

@nikomatsakis nikomatsakis added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jan 14, 2016
@Aatch
Copy link
Contributor

Aatch commented Jan 15, 2016

Making Option a language item was something that I hoped we could avoid, but was probably inevitable anyway. I prefer it to the ad-hoc approach we have right now anyway.

@petrochenkov
Copy link
Contributor

There are two kinds of language items - on the one hand we have "normal" items like Iterator, or Option, or eq_slice, not special-cased in any way. The only thing the compiler has to know about them is their path, then it can use them like usual structs, functions or traits.

On the other hand we have strange beasts like Drop or UnsafeCell deeply integrated into the compiler and having additional custom restrictions or abilities.

Maybe they can be separated somehow?

@arielb1
Copy link
Contributor

arielb1 commented Jan 15, 2016

@petrochenkov

I am not sure about that. Drop is also not that special itself - the compiler prohibits you from using it normally, but its main specialness is that the compiler calls it from drop glue,

@nrc
Copy link
Member

nrc commented Jan 18, 2016

It only seems worthwhile to merge this RFC if we have a plan for how we'll handle lang items in the HIR lowering step. That seems to be the difficult part of implementing the RFC. Since it isn't part of the RFC, the FCP seems premature (unless we just close the PR).

@pnkfelix
Copy link
Member

So the language team reviewed the situation here and came to the following conclusion:

  1. It is not clear whether we want to make the change suggested here.
  2. Deciding whether we want to make the change suggested here would require deeper thought on how such a change would actually be implemented
    • (There is on the one hand the broad question what would be the plan for the set of lang items and how each are special-cased. There is also the much more narrow question of how just this particular desugaring would be revised to use lang items.)

So, we don't want to accept this RFC without having more discussion of the implementation details.

But we also do not think that this RFC thread is the right place for such a discussion of hypothetical implementation details.

There are two places that team members asserted would be better spots for such a discussion:

  1. One could put up a PR against rust-lang/rust with a concrete set of changes. Then we would discuss those changes. (Doing this is in some ways ideal, since there will be no ambiguity about what we are discussing. However, it does run the risk that the team may decide "no, this PR is not quite what we want for the compiler", and closing the PR.
  2. One could put up a post on internals.rust-lang.org to discuss how such an implementation should be made. (That is a better venue for that sort of off-the-cuff discussion, rather than the rather formal process that the RFC protocol follows.)

@pnkfelix
Copy link
Member

(closing; see previous comment for why.)

@pnkfelix pnkfelix closed this Jan 21, 2016
@apasel422 apasel422 deleted the for-loop-lang-items branch January 21, 2016 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-compiler Relevant to the compiler team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.