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

Correctly encode item visibility in metadata #8365

Closed

Conversation

alexcrichton
Copy link
Member

This fixes private statics and functions from being usable cross-crates.

This addresses some concerns about statics being public by default by accident from #8122

@alexcrichton
Copy link
Member Author

There are two-ish pain points of this request

  • Metadata becomes a bit larger because visibility is now also encoded into the metadata for items (it wasn't previously that I could find)
  • The error message for using a private method cross-crate has gone from saying that it's a private method to saying that it's an unresolved name.

@sanxiyn
Copy link
Member

sanxiyn commented Aug 8, 2013

Can the error message be improved later, or is that difficult?

@alexcrichton
Copy link
Member Author

While not impossible, I didn't see a clear way to do them. When I think about it I don't think the privacy checker should exist in its current form today. Right now it runs on the def_id of an invocation and bails out if the item itself is private, but this needs to take into account different scopes of information.

Right now, it'd be easiest to shove this check into resolve, but I think resolve doesn't really need to get bigger. One awesome thing which could be done is to completely remove the idea of privacy from resolve and completely leave it up to the privacy checker. The privacy checker would then have to be aware of the module hierarchy, but I don't think that's really too hard.

@pcwalton, what do you think about moving privacy completely into privacy.rs? That way resolve would resolve everything regardless of whether it's public or private, and then the privacy check pass would be solely responsible for gating whether an access is allowed or not. I'm not quite sure how feasible that would be, but it would be nice from an implementation perspective...

Regardless, the error message I think is difficult to improve now, but I like the idea of improving it later on.

@graydon
Copy link
Contributor

graydon commented Aug 19, 2013

It looks good to me but I'm a little worried about reviewing anything in resolve. r? @pcwalton perhaps?

@pcwalton
Copy link
Contributor

@alexcrichton Yes, I want to move privacy into privacy.rs. Resolve shouldn't know about privacy. (It may change the semantics of use foo::* somewhat but I don't really care; resolve's privacy stuff is too hairy and should be ripped out.)

This fixes private statics and functions from being usable cross-crates
@alexcrichton
Copy link
Member Author

r=pcwalton

@catamorphism
Copy link
Contributor

@alexcrichton The test failure looks legit.

@alexcrichton
Copy link
Member Author

I thought about this a bit, and I'm going to close this for now. I don't want to make resolve any more complicated, so I'm going to try to purge any notion of privacy from resolve and see if I can't re-do this.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 24, 2013
This fixes private statics and functions from being usable cross-crates, along
with some bad privacy error messages. This is a reopening of rust-lang#8365 with all the
privacy checks in privacy.rs instead of resolve.rs (where they should be
anyway).

These maps of exported items will hopefully get used for generating
documentation by rustdoc

Closes rust-lang#8592
bors added a commit that referenced this pull request Sep 25, 2013
…walton

This fixes private statics and functions from being usable cross-crates, along
with some bad privacy error messages. This is a reopening of #8365 with all the
privacy checks in privacy.rs instead of resolve.rs (where they should be
anyway).

These maps of exported items will hopefully get used for generating
documentation by rustdoc

Closes #8592
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 10, 2022
…=camsteffen

Add `explicit_write` suggestions for `write!`s with format args

changelog: Add [`explicit_write`] suggestions for `write!`s with format args

Fixes rust-lang#4542

```rust
writeln!(std::io::stderr(), "macro arg {}", one!()).unwrap();
```

Now suggests:

```
error: use of `writeln!(stderr(), ...).unwrap()`
  --> $DIR/explicit_write.rs:36:9
   |
LL |         writeln!(std::io::stderr(), "macro arg {}", one!()).unwrap();
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `eprintln!("macro arg {}", one!())`
```

---------

r? `@camsteffen` (again, sorry 😛) for the `FormatArgsExpn` change

Before this change `inputs_span` returned a span pointing to just `1` in

```rust
macro_rules! one {
    () => { 1 };
}

`writeln!(std::io::stderr(), "macro arg {}", one!()).unwrap();`
```

And the `source_callsite` of that span didn't include the format string, it was just `one!()`
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.

6 participants