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

Printing function address is confusing due to per-function ZST "singleton type" #75239

Closed
tesuji opened this issue Aug 7, 2020 · 17 comments · Fixed by #76269
Closed

Printing function address is confusing due to per-function ZST "singleton type" #75239

tesuji opened this issue Aug 7, 2020 · 17 comments · Fixed by #76269
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@tesuji
Copy link
Contributor

tesuji commented Aug 7, 2020

I tried this code: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=61fe688172759b7756e84751557e39ea

use std::mem::size_of;
fn bar() -> usize { 42 }
type Left = [u8; 16];
println!("{:p}", size_of::<Left>);
println!("{:p}", bar);

I expected to see this happen:
The compilation succeed or guide me to

  • (1) cast the fn to *const () with as.
  • (2) borrow the fn: &size_of::<Left>. But in this case the addresses printed are the same (what?).
    0x55867559f000
    0x55867559f000
    

Instead, this happened:
Cryptic error message:

error[E0277]: the trait bound `fn() -> usize {std::mem::size_of::<[u8; 16]>}: std::fmt::Pointer` is not satisfied
 --> src/main.rs:6:22
  |
6 |     println!("{:p}", size_of::<Left>);
  |                      ^^^^^^^^^^^^^^^ the trait `std::fmt::Pointer` is not implemented for `fn() -> usize {std::mem::size_of::<[u8; 16]>}`
  |
  = note: required by `std::fmt::Pointer::fmt`
  = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

There is impl<usize> Pointer for fn() -> usize: https://doc.rust-lang.org/nightly/std/fmt/trait.Pointer.html#impl-Pointer-3 .
It took me minutes before I found the workaround. But it still surprised me.

Meta

rustc --version --verbose:

rustc 1.47.0-nightly (22ee68dc5 2020-08-05)
binary: rustc
commit-hash: 22ee68dc586440f96b76b32fbd6087507c6afdb9
commit-date: 2020-08-05
host: x86_64-unknown-linux-gnu
release: 1.47.0-nightly
LLVM version: 10.0
@tesuji tesuji added the C-bug Category: This is a bug. label Aug 7, 2020
@jonas-schievink jonas-schievink added A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. and removed C-bug Category: This is a bug. labels Aug 7, 2020
@tesuji tesuji changed the title Printing fn pointer is hard Printing fn pointer address is hard Aug 7, 2020
@JohnTitor JohnTitor added D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 8, 2020
@RalfJung
Copy link
Member

RalfJung commented Aug 12, 2020

borrow the fn: &size_of::

This creates a reference-to-function-pointer (type &fn()), a double indirection. So it is not what you want.

Cryptic error message:

The error could possibly be improved, and the entire situation documented better, but I think getting an error is expected here. bar is not a function pointer, it is an anonymous "singleton type" (of size 0) indicating exactly the function it was created from. This type coerces to a function pointer, but println! cannot know what type to coerce it to.

Printing function pointers is easy (well, if they are not polymorphic via for), but printing these function singleton ZSTs is hard.

@RalfJung RalfJung changed the title Printing fn pointer address is hard Printing function address is confusing due to per-function ZST "singleton type" Aug 12, 2020
@RalfJung
Copy link
Member

It might also be worth linting against &function.

@RalfJung
Copy link
Member

#75477 helps with the docs. But to close this issue, it would probably make sense to write a lint against &expr where expr is the name of a function.

That should be a matter of adding a new LateLintPass in librustc_lint/builtin.rs which checks all expressions for being AddrOf, with the referent being a Path. That path we can pass to qpath_res and then call opt_def_id to get a DefId. Now we just need to check if that refers to a function: pass the DefId to hir::map::get and call fn_decl.

At least I hope that makes sense, someone stop me if this is all wrong. ;)

@RalfJung RalfJung added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Aug 13, 2020
tmandry added a commit to tmandry/rust that referenced this issue Aug 14, 2020
Expand function pointer docs

Be more explicit in the ABI section, and add a section on how to obtain a function pointer, which can be somewhat confusing.

Cc rust-lang#75239
@RalfJung RalfJung added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label Aug 17, 2020
@ayrtonm
Copy link
Contributor

ayrtonm commented Aug 19, 2020

I'd like to help write this lint. The default level for the lint should be warn right?
@rustbot claim

@RalfJung
Copy link
Member

Yeah, warn feels like a reasonable start.

@RalfJung RalfJung added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 19, 2020
@RalfJung
Copy link
Member

RalfJung commented Aug 19, 2020

Nominated for lang team discussion. It seems we usually ask for lints to be approved before shipping them, so let's get that process started.

@ayrtonm sorry for this, I wasn't aware of this part of the process. You can still go ahead and implement this if you want, but depending on what the lang team decides, we should submit this lint to clippy instead of the compiler itself. That might require minor adjustments, but the main work you are putting in and the testcases will be useful either way. :)

@jyn514
Copy link
Member

jyn514 commented Aug 19, 2020

As a point of interest, this would have been extremely useful in helping debug a segfault in https://gitlab.com/YottaDB/Lang/YDBRust/-/blob/447c641fcf6945d81d6947fcd9a6a8b34b40537f/src/simple_api/mod.rs#L1891. The original code used ydb_lock_st, not ydb_lock_st as *const ().

@RalfJung
Copy link
Member

@jyn514

Without the as cast, transmute will return 1_usize and ydb_call will subsequently segfault.

Without the as cast, this should not compile as transmute checks that the sizes match.
I assume what you describe happens when using &ydb_lock_st, i.e., not just remove the as but also add a &.

@jyn514
Copy link
Member

jyn514 commented Aug 19, 2020

Unfortunately I never committed the broken code so I'm not sure what I had originally ... the closest thing is https://gitlab.com/YottaDB/Lang/YDBRust/-/commit/223b86ac99117672ec8ae8d5a70b4e4da691becf#c03d74036e9573cf34d6119f0c039f06492d3997_1581_1587 which doesn't compile. Sorry for the noise.

@ayrtonm
Copy link
Contributor

ayrtonm commented Aug 19, 2020

@ayrtonm sorry for this, I wasn't aware of this part of the process. You can still go ahead and implement this if you want, but depending on what the lang team decides, we should submit this lint to clippy instead of the compiler itself. That might require minor adjustments, but the main work you are putting in and the testcases will be useful either way. :)

no worries. I already have the lint implemented, but in that case I'll wait until they discuss to write the tests

@RalfJung
Copy link
Member

I think trasmute(&ydb_lock_st) should compile. That's no noise, it seems very relevant. :)

@joshtriplett
Copy link
Member

@rfcbot poll T-lang Should we add a lint for taking a reference to a fn?

@rfcbot
Copy link

rfcbot commented Aug 20, 2020

Team member @joshtriplett has asked teams: T-lang, for consensus on:

Should we add a lint for taking a reference to a fn?

@scottmcm
Copy link
Member

I think I agree here, but to confirm: this isn't actually about linting the taking of a reference to a place of fn type, but just about using & on the voldemort-typed function name constants, right?

@RalfJung
Copy link
Member

using & on the voldemort-typed function name constants

Yes.

@nikomatsakis
Copy link
Contributor

Discussed in a (rather small) @rust-lang/lang meeting today:

Those present felt a lint would make sense. If somebody wanted to go ahead and try to author one, please feel free and nominate the resulting PR for discussion.

@RalfJung
Copy link
Member

@ayrtonm all right, so if you want to open a PR, everything is settled for that. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants