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

Add support for `const unsafe? extern fn` #64906

Merged
merged 3 commits into from Oct 7, 2019

Conversation

@Aaron1011
Copy link
Contributor

Aaron1011 commented Sep 29, 2019

This works just as you might expect - an const extern fn is a const fn that is callable from foreign code.

Currently, panicking is not allowed in consts. When rust-lang/rfcs#2345 (#51999) is stabilized, then panicking in an const extern fn will produce a compile-time error when invoked at compile time, and an abort when invoked at runtime.

Since this is extending the language (we're allowing the const keyword in a new context), I believe that this will need an FCP. However, it's a very minor change, so I didn't think that filing an RFC was necessary.

This will allow libc (and other FFI crates) to make many functions const, without having to give up on making them extern as well.

Tracking issue: #64926.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Sep 29, 2019

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Sep 30, 2019

src/libsyntax/parse/parser/item.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser/item.rs Outdated Show resolved Hide resolved
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Sep 30, 2019

Since this is extending the language (we're allowing the const keyword in a new context), I believe that this will need an FCP. However, it's a very minor change, so I didn't think that filing an RFC was necessary.

This should definitely be feature gated (pre-expansion, see #64672 for examples) and not insta-stabilized. Please create a new tracking issue and think add it to the list in #57563.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Sep 30, 2019

@Centril: I've added a feature gate. changed the grammar, and created a tracking issue.

src/libsyntax/parse/mod.rs Outdated Show resolved Hide resolved
src/libsyntax/feature_gate/active.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser/item.rs Outdated Show resolved Hide resolved
src/test/ui/parser/removed-syntax-extern-const-fn.rs Outdated Show resolved Hide resolved
src/test/ui/reify-intrinsic.stderr Outdated Show resolved Hide resolved
src/test/ui/consts/const-fn-extern.rs Outdated Show resolved Hide resolved
src/test/ui/consts/const-fn-extern.rs Outdated Show resolved Hide resolved
@Centril Centril changed the title Add support for 'extern const fn' Add support for `const unsafe? extern fn` Sep 30, 2019
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Oct 1, 2019

@Centril I've added more tests

src/libsyntax/parse/mod.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser/item.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser/item.rs Outdated Show resolved Hide resolved
src/libsyntax/parse/parser/item.rs Show resolved Hide resolved
@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Oct 1, 2019

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-10-01T17:26:56.5714304Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-01T17:26:56.5912777Z ##[command]git config gc.auto 0
2019-10-01T17:26:56.6001729Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-01T17:26:56.6055279Z ##[command]git config --get-all http.proxy
2019-10-01T17:26:56.6210138Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64906/merge:refs/remotes/pull/64906/merge
---
2019-10-01T17:34:41.6370491Z     Checking fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
2019-10-01T17:34:43.3813655Z error: unexpected token: `.`
2019-10-01T17:34:43.3814556Z     --> src/libsyntax/parse/parser/item.rs:1347:21
2019-10-01T17:34:43.3814922Z      |
2019-10-01T17:34:43.3815186Z 1347 |                     .struct_span_err(self.token.span, "extern items cannot be `const`");
2019-10-01T17:34:43.3815475Z 
2019-10-01T17:34:43.3921616Z error: aborting due to previous error
2019-10-01T17:34:43.3922270Z 
2019-10-01T17:34:43.4022654Z error: could not compile `syntax`.
---
2019-10-01T17:34:43.4117505Z == clock drift check ==
2019-10-01T17:34:43.4133956Z   local time: Tue Oct  1 17:34:43 UTC 2019
2019-10-01T17:34:43.6918782Z   network time: Tue, 01 Oct 2019 17:34:43 GMT
2019-10-01T17:34:43.6920065Z == end clock drift check ==
2019-10-01T17:34:44.8227757Z ##[error]Bash exited with code '1'.
2019-10-01T17:34:44.8304485Z ##[section]Starting: Checkout
2019-10-01T17:34:44.8306059Z ==============================================================================
2019-10-01T17:34:44.8306125Z Task         : Get sources
2019-10-01T17:34:44.8306162Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Oct 1, 2019

@Centril: I've added more tests, and implemented parsing recovering for extern blocks.

This works just as you might expect - an 'extern const fn' is a 'const
fn' that is callable from foreign code.

Currently, panicking is not allowed in consts. When RFC 2345 is
stabilized, then panicking in an 'extern const fn' will produce a
compile-time error when invoked at compile time, and an abort when
invoked at runtime.

Since this is extending the language (we're allowing the `const` keyword
in a new context), I believe that this will need an FCP. However, it's a
very minor change, so I didn't think that filing an RFC was necessary.

This will allow libc (and other FFI crates) to make many functions
`const`, without having to give up on making them `extern` as well.
@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/extern-const-fn branch from 99095f4 to 73b50d2 Oct 2, 2019
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Oct 2, 2019

Thanks! -- Implementation looks good now.

@rust-lang/lang N.B. this adds support for const unsafe? extern ABI? fn under a feature gate const_extern_fn. The changes are syntactic in nature and the semantic changes mainly just fall out. I'll r+ this since I think it's fine to land for experimentation purposes and the discussion can progress on the tracking issue. However, I think we may eventually want a stabilization RFC that takes into account all of the discussion thus far and elaborates on semantics, use-cases, etc.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 2, 2019

📌 Commit 84b680f has been approved by Centril

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 2, 2019
…r=Centril

Add support for `const unsafe? extern fn`

This works just as you might expect - an `const extern fn` is a `const fn` that is callable from foreign code.

Currently, panicking is not allowed in `const`s. When rust-lang/rfcs#2345 (rust-lang#51999) is stabilized, then panicking in an `const extern fn` will produce a compile-time error when invoked at compile time, and an abort when invoked at runtime.

Since this is extending the language (we're allowing the `const` keyword in a new context), I believe that this will need an FCP. However, it's a very minor change, so I didn't think that filing an RFC was necessary.

This will allow libc (and other FFI crates) to make many functions `const`, without having to give up on making them `extern` as well.

Tracking issue: rust-lang#64926.
bors added a commit that referenced this pull request Oct 2, 2019
Rollup of 7 pull requests

Successful merges:

 - #64749 (Fix most remaining Polonius test differences)
 - #64906 (Add support for `const unsafe? extern fn`)
 - #64959 (syntax: improve parameter without type suggestions)
 - #64975 (Implement Clone::clone_from for LinkedList)
 - #64993 (BacktraceStatus: add Eq impl)
 - #64998 (Filter out RLS output directories on tidy runs)
 - #65010 (Compare `primary` with maximum of `children`s' line num instead of dropping it)

Failed merges:

r? @ghost
@@ -1486,6 +1486,15 @@ impl<'a> Parser<'a> {
Ok(())
}

/// Parses `extern` followed by an optional ABI string, or nothing.
fn parse_extern_abi(&mut self) -> PResult<'a, Abi> {

This comment has been minimized.

Copy link
@Centril

Centril Oct 3, 2019

Member

Btw, I was going over parser/ty.rs and I noticed that parse_ty_bare_fn has the exact same logic. Would be good to de-duplicate that.

bors added a commit that referenced this pull request Oct 3, 2019
Make re-export collection deterministic

Fixes #65036

Previously, we were using an `FxHashMap` to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR #64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.

See #65042 for a long-term strategy to detect this kind of issue
bors added a commit that referenced this pull request Oct 3, 2019
Make re-export collection deterministic

Fixes #65036

Previously, we were using an `FxHashMap` to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR #64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.

See #65042 for a long-term strategy to detect this kind of issue
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Oct 5, 2019
Previously, we were using an `FxHashMap` to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR rust-lang#64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.

See rust-lang#65042 for a long-term strategy to detect this kind of issue
bors added a commit that referenced this pull request Oct 6, 2019
…nkov

Make re-export collection deterministic

Fixes #65036

Previously, we were using an `FxHashMap` to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR #64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.

See #65042 for a long-term strategy to detect this kind of issue
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Oct 6, 2019

@Centril : This is ready to be merged now that the non-determinism has been fixed

@Centril Centril closed this Oct 6, 2019
@Centril Centril reopened this Oct 6, 2019
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Oct 6, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 6, 2019

📌 Commit a4cad41 has been approved by Centril

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 7, 2019

⌛️ Testing commit a4cad41 with merge 4ac4809...

bors added a commit that referenced this pull request Oct 7, 2019
Add support for `const unsafe? extern fn`

This works just as you might expect - an `const extern fn` is a `const fn` that is callable from foreign code.

Currently, panicking is not allowed in `const`s. When rust-lang/rfcs#2345 (#51999) is stabilized, then panicking in an `const extern fn` will produce a compile-time error when invoked at compile time, and an abort when invoked at runtime.

Since this is extending the language (we're allowing the `const` keyword in a new context), I believe that this will need an FCP. However, it's a very minor change, so I didn't think that filing an RFC was necessary.

This will allow libc (and other FFI crates) to make many functions `const`, without having to give up on making them `extern` as well.

Tracking issue: #64926.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 7, 2019

☀️ Test successful - checks-azure
Approved by: Centril
Pushing 4ac4809 to master...

@bors bors added the merged-by-bors label Oct 7, 2019
@bors bors referenced this pull request Oct 7, 2019
@bors bors merged commit a4cad41 into rust-lang:master Oct 7, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191006.62 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@Aaron1011 Aaron1011 deleted the Aaron1011:feature/extern-const-fn branch Oct 7, 2019
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Oct 9, 2019

@Aaron1011 Could we also get a test making sure that we properly report an error when calling a const fn using the wrong ABI, such as calling an extern "C" const fn with the call site assuming it is an extern "Rust" const fn? The checks for that should already exist in the engine (for standalone Miri).

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Oct 9, 2019

@RalfJung Excellent point; added a check-box to the tracking issue.

XiangQingW added a commit to XiangQingW/rust that referenced this pull request Oct 13, 2019
Previously, we were using an `FxHashMap` to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR rust-lang#64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.

See rust-lang#65042 for a long-term strategy to detect this kind of issue
choller added a commit to choller/rust that referenced this pull request Oct 17, 2019
Previously, we were using an `FxHashMap` to collect module re-exports.
However, re-exports end up getting serialized into crate metadata, which
means that metadata generation was non-deterministic. This resulted in
spurious error messages changes (e.g. PR rust-lang#64906) due to pretty-printing
implicitly depending on the order of re-exports when computing the
proper path to show to the user.

See rust-lang#65042 for a long-term strategy to detect this kind of issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.