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

Correct errors in the reference of extern functions definitions and declarations #652

Merged
merged 15 commits into from
Aug 22, 2019

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Aug 14, 2019

The current reference of extern functions definitions is incorrect. It doesn't take into account that all "normal" Rust functions are extern "Rust" fn.

The section about external blocks specifies that only "non-Rust functions" (whatever that means) can be declared in external blocks. This is incorrect, since both an extern "C" Rust function, and a function with the "Rust" ABI, can be declared in external blocks.

src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 15, 2019

@eddyb would you be able to review these changes as well ? Particularly, I've added that some ABIs are equivalent to "Rust" and documented this for some of them. Would like to triple-check that this is indeed actually the case.

@gnzlbg gnzlbg changed the title Correct errors in the reference of extern functions definitions Correct errors in the reference of extern functions definitions and declarations Aug 15, 2019
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 15, 2019

@Centril I also have the feeling that talking about function definitions/declarations is maybe too "C/C++ speak".

Is there a better and clearer way to talk about these in "Rust speak" ? The only thing that comes to mind is to consistently use "function item" for "definitions" (fn foo() {}), "external function item" for "declarations" (extern { fn foo(); }), and "extern-qualified function item" or "function item with an extern qualifier" for a "definition" that uses the extern qualifier (extern fn foo() {}). However, I feel that it is too easy to confuse "external function item" with "extern-qualified function item" when discussing this part of the language.

src/items/external-blocks.md Outdated Show resolved Hide resolved
src/items/external-blocks.md Outdated Show resolved Hide resolved
src/items/external-blocks.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Aug 15, 2019

@Centril I also have the feeling that talking about function definitions/declarations is maybe too "C/C++ speak".

See https://doc.rust-lang.org/nightly/reference/items/traits.html#traits
You'll note:

TraitFunc :
      TraitFunctionDecl ( ; | BlockExpression )

I think it's fine to use "declaration" for function signatures and definitions for declarations enhanced with a block expression.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 15, 2019

I think it's fine to use "declaration" for function signatures and definitions for declarations enhanced with a block expression.

Makes sense, I'll keep the text that uses definition/declarations then.

Maybe we could open an issue about updating the extern-block section to use "ExternalFunctionDeclaration" instead of "ExternalFunctionItem". I am not sure if such a change makes sense, or whether there are other places where we might want to take this into account. Probably should be consistent with the grammar as well, although maybe the grammar calls these something different, no idea.

@Centril
Copy link
Contributor

Centril commented Aug 15, 2019

Let's do that in a follow up if so; we'll need to make sure that we have no stale references to/in the grammar if we change.

External blocks form the basis for Rust's foreign function interface.
Declarations in an external block describe symbols in external, non-Rust
libraries.
External blocks provide _declarations_ of items that are not _defined_ in the
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be more explicit that these are "imports" of a sort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to clarify that.

src/items/external-blocks.md Outdated Show resolved Hide resolved
gnzlbg and others added 2 commits August 15, 2019 10:54
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
src/items/functions.md Outdated Show resolved Hide resolved
libraries.
External blocks provide _declarations_ of items that are not _defined_ in the
current crate and are the basis of Rust's foreign function interface. These are
sort of like unchecked imports.
Copy link
Contributor

@Centril Centril Aug 15, 2019

Choose a reason for hiding this comment

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

Nit: "sort of like" reads rather like spoken English (not something you'd find in an IEEE paper, which is the sort of style we should shoot for imo...).

Copy link
Contributor Author

@gnzlbg gnzlbg Aug 15, 2019

Choose a reason for hiding this comment

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

I didn't intend to take @eddyb feedback literally here, but that's the best I could come up with.

These blocks declare that something exists in a particular way. If you "use" (call, access, etc.) it and it doesn't exist, or it exists in a way that's different than how you declared it: boom. If you don't actually use it, whether it exists doesn't matter, and if it doesn't, that's ok.

OTOH, to me, an import is an operation that brings something into scope that exists. If it doesn't exist, then you can't import it. So I'm not sure what would be the best way to make an analogy with imports here.

Maybe something like

Declarations are similar to imports, in that they bring an item into scope. The main difference is that an import requires an item to exist, while a declaration states that an item exists with a particular type signature, attributes, etc. at a particular path. In contrast with imports, a declaration only needs to exist when it is "used" (for a function, called, for a static, accessed). This allows, for example, declaring a dynamically-linked function unconditionally and copying its function type around even when the dynamic library that links it isn't actually linked. The function is only required to exist (e.g. the dynamic library to be linked) when the program actually calls the function.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe with an example like this:

extern "C" { fn foo(); }

fn main() {
    let x = foo;
    if symbol_of_foo_not_linked() {
         link_symbol_of_foo();
    }
    unsafe { x() };
}

?

Copy link
Contributor

Choose a reason for hiding this comment

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

@eddyb @ehuss wdyt about the wording ^-- ?

Copy link
Member

Choose a reason for hiding this comment

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

I use "FFI import" to mean something different from what the use item does in Rust, so I'm not comparing the two but rather using the more general meaning of "import".

Also, not sure that where the function comes from in terms of linkage is that important, compared to the fact that it likely isn't even written in Rust, hence "FFI".

src/items/functions.md Outdated Show resolved Hide resolved
Havvy
Havvy previously requested changes Aug 16, 2019
Copy link
Contributor

@Havvy Havvy left a comment

Choose a reason for hiding this comment

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

A couple small nits; Given the number of eyes and the number of changes, I'd like to see this land, and a new PR open for more changes. Don't let perfect be the enemy of good.

src/items/external-blocks.md Outdated Show resolved Hide resolved
src/items/external-blocks.md Outdated Show resolved Hide resolved
src/items/functions.md Show resolved Hide resolved
extern "Rust" fn foo() {}
```

Functions in Rust can be called by foreign code, and using an ABI that
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a run-on sentence, or at least feels like one. Also, "Rust's functions". And what does "foreign code" here mean differently than code from other programming languages?

Copy link
Contributor

Choose a reason for hiding this comment

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

Rust code exposed with a C ABI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to improve this.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 16, 2019

cc @Gankra

gnzlbg and others added 2 commits August 16, 2019 14:46
Co-Authored-By: Ryan Scheel <Ryan.havvy@gmail.com>
Co-Authored-By: Ryan Scheel <Ryan.havvy@gmail.com>
@Gankra
Copy link
Contributor

Gankra commented Aug 16, 2019

Since most of the discussion is collapsed and there's 14 commits of history, it's hard for me to tell what happened here.

I just want to get a clarification as to what has happened here with rust-call/rust-intrinsic/platform-intrinsic. They were never properly documented so it's unclear to be exactly what they imply (rust-intrinsic is only clearish one to me).

Are these things stricken from the record as pure perma-unstable implementation details that no one should ever have to think about? Is there another document somewhere that stdlib and rustc devs can refer to for the meaning of them? (rustc/std should be the most active users of a reference, after all)

In particular it is slightly interesting to me as to whether callers of rust-intrinsic and platform-intrinsic methods can rely on them never unwinding (I assume rust-call must in fact be allowed to unwind).

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 16, 2019

I just want to get a clarification as to what has happened here with rust-call/rust-intrinsic/platform-intrinsic. [...] Are these things stricken from the record as pure perma-unstable implementation details that no one should ever have to think about?

IIUC, the reference is only intended (at least for the time being), to document stable Rust. All unstable Rust information should be documented in the Unstable Book.

Now, the information that this PR removes is about perma-unstable features that are not intended to ever be stabilized. Most people, including most of those hacking in the standard library, shouldn't have to know about this. Knowledge about these is only required if you are touching standard library code that requires special compiler support. So the ideal place to document these would probably be the rustc book.

The information that this PR removes wasn't great either, and it is a bit out-of-date. For example, most "platform-intrinsic" (thousands of them) were removed from rustc a while ago, and now AFAICT this ABI is only used for generic SIMD intrinsics... for no good reason. As @eddyb pointed out here, we probably want to just change these to be extern "Rust" and handle their "magicness" somehow else.

In particular it is slightly interesting to me as to whether callers of rust-intrinsic and platform-intrinsic methods can rely on them never unwinding

They can't. These are essentially extern "Rust" which can unwind. Whether these unwind or not, would depend on the concrete intrinsic that we are dealing with (I don't have any concrete intrinsic in mind that actually unwinds, but this doesn't mean there isn't one).

@Gankra
Copy link
Contributor

Gankra commented Aug 16, 2019

I expect whatever primitive operation integer addition lowers to is a fine canonical example of an unwinding intrinsic.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 22, 2019

@Havvy @Centril I'm uncertain of which changes are still required here.

Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
@Centril Centril merged commit 2b29a56 into rust-lang:master Aug 22, 2019
bors added a commit to rust-lang/rust that referenced this pull request Sep 4, 2019
Update cargo, books

## cargo

8 commits in 22f7dd0495cd72ce2082d318d5a9b4dccb9c5b8c..fe0e5a48b75da2b405c8ce1ba2674e174ae11d5d
2019-08-27 16:10:51 +0000 to 2019-09-04 00:51:27 +0000
- Rename `--all` to `--workspace` (rust-lang/cargo#7241)
- Basic standard library support. (rust-lang/cargo#7216)
- Allow using 'config.toml' instead of just 'config' files. (rust-lang/cargo#7295)
- Retry on SSL Connect Error. (rust-lang/cargo#7318)
- minimal-copy `deserialize` for `InternedString` (rust-lang/cargo#7310)
- Fix typo in cargo vendor examples (rust-lang/cargo#7320)
- Fixes around multiple `[patch]` per crate (rust-lang/cargo#7303)
- Improve error messages on mkdir failure (rust-lang/cargo#7306)

## reference

7 commits in d191a0c..090c015
2019-08-15 08:42:23 +0200 to 2019-09-03 13:59:28 -0700
- Fix rust-lang/reference#664: Review Oxford comma usage. (rust-lang/reference#668)
- Fix some links. (rust-lang/reference#667)
- Remove trait object warning. (rust-lang/reference#666)
- Specify pattern types in `let` statements and `for` expressions (rust-lang/reference#663)
- Fix loop expression link. (rust-lang/reference#662)
- async-await initial reference material (rust-lang/reference#635)
- Correct errors in the reference of extern functions definitions and declarations (rust-lang/reference#652)

## rust-by-example

1 commits in 580839d90aacd537f0293697096fa8355bc4e673..e76be6b2dc84c6a992e186157efe29d625e29b94
2019-08-17 23:17:50 -0300 to 2019-09-03 07:42:26 -0300
- Change link to russian translation repository (rust-lang/rust-by-example#1245)

## embedded-book

1 commits in 432ca26686c11d396eed6a59499f93ce1bf2433c..5ca585c4a7552efb546e7681c3de0712f4ae4fdc
2019-08-09 23:20:22 +0000 to 2019-08-27 13:39:14 +0000
- Fixup book CI  (rust-embedded/book#205)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants