Add a crate keyword and allow it to link external crates #12017

flaper87 · 2014-02-03T22:59:39Z

The first setp for #9880 is to add a new crate keyword. This PR does exactly that. I took a chance to refactor parse_item_foreign_mod and I broke it down into 2 separate methods to isolate each feature.

The next step will be to push a new stage0 snapshot and then get rid of all extern mod around the code.

flaper87 · 2014-02-03T23:01:55Z

@huonw r?

This is a first iteration for this change! It seems pretty complete, although I would appreciate a detailed review on both methods since the re-factor could have introduced some regressions.

thestinger · 2014-02-03T23:07:47Z

Was it decided to go with crate in a meeting? I'm opposed to changing this and there hasn't been a response to my comment on the issue. The prevailing opinion may be that extern mod is confusing but it is not the only opinion and no one has stated why calling it crate (Rust-specific term unknown to outsiders) will be better than calling it an externally defined module.

A C library is certainly not considered a crate, and the plan for supporting libclang-based symbol resolution for native libraries was to add an attribute to an extern mod statement providing the header name.

alexcrichton · 2014-02-03T23:09:30Z

@thestinger, you may be interested in these minutes: https://github.com/mozilla/rust/wiki/Meeting-weekly-2013-12-17#wiki-extern-mod---extern-crate

thestinger · 2014-02-03T23:12:19Z

Ah, I wasn't aware it had been discussed - nevermind then.

huonw · 2014-02-03T23:30:14Z

src/libsyntax/parse/parser.rs

@@ -4608,12 +4587,33 @@ impl Parser {
                                        visibility,
                                        maybe_append(attrs, extra_attrs));
                return IoviItem(item);
-            } else  {
+            } else if self.eat_keyword(keywords::Crate) {


I think this might be better to put this before .parse_opt_abis. i.e.

if self.is_keyword(keywords::Extern) { let next_is_mod = self.eat_keyword(keywords::Mod); if self.eat_keyword(keywords::Crate) || next_is_mod { if next_is_mod { self.span_note(...) } self.parse_item_extern_crate(lo, visibility, attrs); } let opt_abis = self.parse_opt_abis(); if self.eat_keyword(keywords::Fn) { ... } ... }

This means that extern "ABI" crate blah; is an "unexpected token crate" error, which seems reasonable. Although the keeping it the way it is now seems ok-ish.

Also, as I hinted in then code above above, wouldn't this be better as (something equivalent to) self.eat_keyword(keywords::Crate) || self.eat_keyword(keywords::Mod) rather than duplicating the logic below?

As it is now, it still raises the "ABI" error but I think we should get rid of that. I'll refactor the code based on your suggestions.

flaper87 · 2014-02-05T16:54:31Z

This requiers to change all usages of crate in the code, I'll push a new version of this patch.

huonw · 2014-02-06T00:37:57Z

src/libsyntax/parse/parser.rs

+                return self.parse_item_foreign_mod(lo, opt_abis, visibility, attrs);
+            }
+
+            if opt_abis.is_some() {


Don't we need the "expected fnor{" error here? (i.e. replace this if with just self.span_fatal(self.span, format!("expected ... but found {}", self.this_token_to_str()).)

The else if self.token should handle extern { I'll add an else clause with the error you suggested or re-write this if entirely.

No, I'm talking about the case when it's not a { or fn (i.e. neither case is triggered above): test it with extern blah;. It should emit an error, but it won't.

And yet I again, I should stop doing so many things at the same time. I was talking about the else if self.token not this specific if statement. I agree it should go away, in fact, I already removed it.

emberian · 2014-02-06T17:38:24Z

I would like this to be held off until a meeting, where making crate a contextual keyword can be discussed.

flaper87 · 2014-02-06T18:28:58Z

@cmr I went ahead and worked on this based on @brson comment. The idea is to make it a full keyword for now and then contextualized it later if there's an agreement to do that. #9880 (comment)

brson · 2014-02-11T18:51:56Z

In the meeting today we decided to continue with crate as a full keyword under the assumption that 'crate' is uncommon outside the compiler. We can file followup issues about making crate and in conditional keywords.

Let's please rename uses of crate variables to krate. Sorry this will require a bunch of rewriting.

extern crate is intended to always be used to link to Rust libraries, not C libraries.

alexcrichton · 2014-02-13T00:06:26Z

I asked @cmr to remove his r+ because I realized that this still emits a NOTE for extern mod. This needs to emit an error for extern mod. For bootstrapping purposes, it's ok to allow both, but we don't want to litter the build with NOTE everywhere asking us to do something that can't be done just yet.

flaper87 · 2014-02-13T12:49:00Z

@alexcrichton Commented the span_err out and I'll un-comment it as soon as the snapshot is up. r?

alexcrichton · 2014-02-13T19:19:50Z

Looks like this failed due to a legitimate reason

flaper87 · 2014-02-13T19:51:13Z

@alexcrichton damn, I forgot to update the error message. Done: r? p=?

This patch replaces all `crate` usage with `krate` before introducing the new keyword. This ensures that after introducing the keyword, there won't be any compilation errors. krate might not be the most expressive substitution for crate but it's a very close abbreviation for it. `module` was already used in several places already.

This patch adds a new keyword `crate` which is intended to replace mod in the context of `extern mod` as part of the issue rust-lang#9880. The patch doesn't replace all `extern mod` cases since it is necessary to first push a new snapshot 0. The implementation could've been less invasive than this. However I preferred to take this chance to split the `parse_item_foreign_mod` method and pull the `extern crate` part out of there, hence the new method `parse_item_foreign_crate`.

This patch gets rid of ObsoleteExternModAttributesInParens and ObsoleteNamedExternModule since the replacement of `extern mod` with `extern crate` avoids those cases and raises different errors. Both have been around for at least a version which makes this a good moment to get rid of them.

The first setp for #9880 is to add a new `crate` keyword. This PR does exactly that. I took a chance to refactor `parse_item_foreign_mod` and I broke it down into 2 separate methods to isolate each feature. The next step will be to push a new stage0 snapshot and then get rid of all `extern mod` around the code.

minor: Bump deps

huonw reviewed Feb 3, 2014
View reviewed changes

huonw reviewed Feb 6, 2014
View reviewed changes

liigo mentioned this pull request Feb 8, 2014

Use keyword crate to define crates: pub crate std; #12107

Closed

flaper87 added 3 commits February 13, 2014 20:52

bors closed this Feb 14, 2014

bors merged commit 5deb3c9 into rust-lang:master Feb 14, 2014

chris-morgan added a commit to chris-morgan/ncurses-rs that referenced this pull request Feb 16, 2014

s/extern mod/extern crate/ (rust-lang/rust#12017)

1113380

reima mentioned this pull request Feb 19, 2014

Fixes for compatibility with Rust master Rust-SDL2/rust-sdl2#45

Merged

flaper87 deleted the replace-mod-crate branch March 11, 2014 09:16

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022

Auto merge of rust-lang#12017 - lnicola:bump-deps, r=lnicola

65fbe0a

minor: Bump deps

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a crate keyword and allow it to link external crates #12017

Add a crate keyword and allow it to link external crates #12017

flaper87 commented Feb 3, 2014

flaper87 commented Feb 3, 2014

thestinger commented Feb 3, 2014

alexcrichton commented Feb 3, 2014

thestinger commented Feb 3, 2014

huonw Feb 3, 2014

flaper87 Feb 4, 2014

flaper87 commented Feb 5, 2014

huonw Feb 6, 2014

flaper87 Feb 6, 2014

huonw Feb 6, 2014

flaper87 Feb 6, 2014

emberian commented Feb 6, 2014

flaper87 commented Feb 6, 2014

brson commented Feb 11, 2014

alexcrichton commented Feb 13, 2014

flaper87 commented Feb 13, 2014

alexcrichton commented Feb 13, 2014

flaper87 commented Feb 13, 2014

Add a crate keyword and allow it to link external crates #12017

Add a crate keyword and allow it to link external crates #12017

Conversation

flaper87 commented Feb 3, 2014

flaper87 commented Feb 3, 2014

thestinger commented Feb 3, 2014

alexcrichton commented Feb 3, 2014

thestinger commented Feb 3, 2014

huonw Feb 3, 2014

Choose a reason for hiding this comment

flaper87 Feb 4, 2014

Choose a reason for hiding this comment

flaper87 commented Feb 5, 2014

huonw Feb 6, 2014

Choose a reason for hiding this comment

flaper87 Feb 6, 2014

Choose a reason for hiding this comment

huonw Feb 6, 2014

Choose a reason for hiding this comment

flaper87 Feb 6, 2014

Choose a reason for hiding this comment

emberian commented Feb 6, 2014

flaper87 commented Feb 6, 2014

brson commented Feb 11, 2014

alexcrichton commented Feb 13, 2014

flaper87 commented Feb 13, 2014

alexcrichton commented Feb 13, 2014

flaper87 commented Feb 13, 2014