search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.9k stars 12.52k forks source link

Tracking issue for RFC 1977: public & private dependencies #44663

Open withoutboats opened 7 years ago

withoutboats commented 7 years ago

Summary

RFC (original, superseded): #1977 RFC: #3516 Cargo tracking issue: https://github.com/rust-lang/cargo/issues/6129 Issues: https://github.com/rust-lang/rust/issues?q=is%3Aissue+is%3Aopen+label%3AF-public_private_dependencies Cargo issues: https://github.com/rust-lang/cargo/issues?q=is%3Aopen+is%3Aissue+label%3AZ-public-dependency Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#public-dependency

This feature enables the ability to track which dependencies are publicly exposed through a library's interface. It has two sides to the implementation: rustc (lint and --extern flag), and cargo (Cargo.toml syntax, and passing --extern flags).

This feature was originally specified in rust-lang/rfcs#1977, but was later down-scoped in rust-lang/rfcs#3516.

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label. Discussion comments will get marked as off-topic or deleted. Repeated discussions on the tracking issue may lead to the tracking issue getting locked.

Unresolved Questions

Steps

Non-blocking further improvements

Changes from RFC

cc @rust-lang/cargo @epage

epage commented 1 year ago

@Eh2406 and I met to talk about the status of this. This represents just the thoughts of the two of us and not the whole cargo team

The pub/private RFC is quite old, predating

And likely other factors that affect it.

We are also concerned about the status of the proposed resolver changes. In addition to likely being long tail for work for this feature

graph TD;
    root-->depA_v1;
    root-->depB_v1;
    depA_v1-->depB_v2;

The hope was that the resolver changes would help with

We propose that someone

Some of the remaining motivations include:

jplatte commented 1 year ago

For a new RFC, something that should IMHO be listed as prior art, maybe also be a comparison point ("why is the proposed solution better than") is https://github.com/davidpdrsn/cargo-public-api-crates.

epage commented 11 months ago

FYI I've created a superseding Pre-RFC

alexcrichton commented 11 months ago

Oh awesome! I left a comment over there, and thanks!

EDIT: this was the wrong thread for this comment and I apologize

linyihai commented 10 months ago

Hey，the unstable feature was existed and I think This new features @epage mentions may be not include the unstable feature. Is it right? I'm really looking forward to contributing to the new features. :)

epage commented 10 months ago

I've updated the tracking issue to account for rust-lang/rfcs#3516.

epage commented 9 months ago

@Muscraft the new RFC didn't cover workspace inheritance. rust-lang/cargo#12817 implemented it so it can be specified in the workspace and the package can override it. Does that sound reasonable?

epage commented 9 months ago

I somewhat lean towards saying the workspace can't specify public because it seems like that is something we don't want people overspecifying. Unsure how bad that'd be for the implementation or what our other support cases are like.

EDIT: looks like we prevent optional from being in the workspace, so we have precedence for that.

djc commented 9 months ago

Yup, I agree workspaces should not be allowed to specify public.

epage commented 9 months ago

@alexcrichton looking closer at this, it seems like it'd be reasonable to block public in workspace.dependencies, like optional. Since you did rust-lang/cargo#12817, any thoughts on this?

djc commented 9 months ago

@epage are you sure you linked the correct issue/PR?

epage commented 9 months ago

Fixed. Forgot what repo I was in.

alexcrichton commented 9 months ago

IIRC one of the motivaitons for blocking optional in workspace dependencies was that it had interactions with how features were calculated and it seemed best to not accidentally implicitly add features to crates via workspace dependencies. In my opinion similar logic isn't applicable to public and it's something I'd personally like to see available at the workspace.dependencies level. My usage of workspace dependencies and public/private is typically along the lines of "this is public for all crates using this" or the opposite.

For example when I pull in anyhow that's because nearly all crates will use anyhow::Result<T> in their APIs. Alternatively crates like libc are likely to be a private dependency.

So while I don't disagree that it would be conservative to block public, I'd personally still like it to avoid the repetition of writing it everywhere. Please feel free to overrule me however!

Another possible interesting case study is default-features. That's a boolean setting for workspace dependencies which is a bit fiddly, but I think that primarily stems from its "default on" behavior as opposed to default-off like other boolean flags like optional. In that sense default-features may be different enough to not help guide public too.

djc commented 9 months ago

I feel like because the hazard is on the side of defaulting to public = true, workspace dependencies and things like that have defaulted to requiring workspace = true explicitly in a package manifest. If the workspace makes a dependency public, that will not be clear when reviewing the package manifest, which seems like it might cause issues.

On the other hand, it might be okay to allow workspace.dependencies that specify workspace = true to inherit the public setting from the workspace too. But, it doesn't seem crazy to suggest that you might want some dependency to be public in maybe some inner-level mostly internal-facing dependencies while you want to hide it from the outer-level public-facing dependency (for easier sharing across a single-organization tree of crates).

Finally, I also feel like there's a case here for saying, we don't create any new problems by denying the use of public at the workspace level now because we can start enabling it later, whereas allowing it now with semantics that turn out to be problematic would be unfortunate.

epage commented 9 months ago

In reflecting back, I wish workspace dependencies only focused on the Source and didn't allow specifying the rest. There are times to keep the rest in sync but I feel like that can lead to false sharing which can cause its own problems and having them explicit would make things clearer / more approachable.

However, the work to get there would take a lot of work and could have some downsides, so I'm not wanting to advocate for it now.

Where that leaves us for this, i'm less sure. I do find it appealing that erroring now leaves the door open in the future for not erroring.

epage commented 9 months ago

@Muscraft and I talked some more and I think we'll not allow public at the workspace level.

Maybe if we had "grouped" inheritance (named, mutually exclusive sets of fields/deps that you can inherit) as that would mean you can then define things by policy rather than globally.

epage commented 9 months ago

Regarding cargo fix support, I was hoping that rustc would give cargo enough information to intercept the warning and add machine-applicable information to it but apparently not:

{
  "reason": "compiler-message",
  "package_id": "cargo-pub-priv 0.1.0 (path+file:///home/epage/src/personal/dump/cargo-pub-priv)",
  "manifest_path": "/home/epage/src/personal/dump/cargo-pub-priv/Cargo.toml",
  "target": {
    "kind": [
      "lib"
    ],
    "crate_types": [
      "lib"
    ],
    "name": "cargo-pub-priv",
    "src_path": "/home/epage/src/personal/dump/cargo-pub-priv/src/lib.rs",
    "edition": "2021",
    "doc": true,
    "doctest": true,
    "test": true
  },
  "message": {
    "rendered": "warning: trait `FooTrait` from private dependency 'foo' in public interface\n  --> src/lib.rs:11:1\n   |\n11 | pub trait UseFoo: foo::FooTrait {}\n   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n",
    "children": [],
    "code": {
      "code": "exported_private_dependencies",
      "explanation": null
    },
    "level": "warning",
    "message": "trait `FooTrait` from private dependency 'foo' in public interface",
    "spans": [
      {
        "byte_end": 189,
        "byte_start": 158,
        "column_end": 32,
        "column_start": 1,
        "expansion": null,
        "file_name": "src/lib.rs",
        "is_primary": true,
        "label": null,
        "line_end": 11,
        "line_start": 11,
        "suggested_replacement": null,
        "suggestion_applicability": null,
        "text": [
          {
            "highlight_end": 32,
            "highlight_start": 1,
            "text": "pub trait UseFoo: foo::FooTrait {}"
          }
        ]
      }
    ]
  }
}

I wonder how receptive the compiler folks would be for adding one-off data like that...

The next best alternative I can think of is for the lint to only be a warning in 2024, rather than an error. However, I had hoped for it to be a warning on stabilization and we decided not to...

epage commented 9 months ago

Really terrible idea that might be just enough to get over the hump for cargo fix: we parse message.message looking for "from private dependency '{name}' in public interface".

epage commented 9 months ago

rust-lang/cargo#13046 added cargo add support, assuming workspace inheritance for public will be removed. If we decide to keep it, we'll need to go back and update it.

epage commented 9 months ago

@linyihai one item that i didn't create an issue for is

verify public is respected recursively

Thats because I'm unsure if this is already covered or not.

linyihai commented 9 months ago

Do we have a test for it?

After Checking the test cases contains cargo-features = ["public-dependency"], I didn't found the scenior.

If not, we should write it

@epge I would be pleasure to add it. But I am not sure how to verify public is respected recursively. To be honest, I'm not quite sure what does public is respected recursively means

epage commented 9 months ago

@epge I would be pleasure to add it. But I am not sure how to verify public is respected recursively. To be honest, I'm not quite sure what does public is respected recursively means

I'm going to give an example that assumes #71043 is resolved. It might be more finicky to create an example without it resolved; I'm unsure.

pub:

pub struct A;

parent has a public dependency on pub

pub use pub::*;

grandparent has a public dependency on parent:

pub use parent::*;

Does how cargo interact with rustc correctly to handle not warn? What other variations might we run into?

jonhoo commented 9 months ago

13046 added cargo add support, assuming workspace inheritance for public will be removed. If we decide to keep it, we'll need to go back and update it.

nit: this should be https://github.com/rust-lang/cargo/pull/13046 — as-written it refers to #13046 on the rust-lang/rust repo.

linyihai commented 8 months ago

RFC did not specify workspace inheritance behavior. With https://github.com/rust-lang/rust/issues/13125, we disallow it.

(nit: the link should be https://github.com/rust-lang/cargo/pull/13125.)

epage commented 8 months ago

Thanks, fixed!

tmandry commented 8 months ago

I think the decision to require a key (public = true) that breaks old cargo versions is a mistake. It shouldn't have been implemented in such a way that it produced a hard error in cargo in the first place, but now many crates in the ecosystem will be prevented from using this highly desirable feature for years.

At the same time I understand that public = true is the most ergonomic option. Can we at least provide an alternative key, for crates that care about MSRV?

epage commented 8 months ago

I can understand the desire for this to not require an MSRV change. Part of the problem is historical, that the original feature was designed to be very different than what we have today. Part of the problem is not require an MSRV bump is very out of the ordinary with the only cases I can think of being package.rust-version and lints.

However, I don't feel a redundant field would be appropriate.

We could consider dropping the nightly requirement now so we at least have a couple release lead time on this problem (with the risk of stabilzation being even further out than that with the state of the compiler side).

I do think this conversation represents a deeper problem: a push towards stagnation due to MSRV. I'm working on the update for the MSRV resolver RFC that I'm hoping will help lay out a better path for MSRV policies that can help push us out of this stagnation, shifting costs to those that need old versions rather than being old Rust versions being a millstone around the communities neck.

tmandry commented 8 months ago

I started on this because I thought public/private deps would weigh on the ability of a library crate to be linked dynamically within a larger build graph, but I don't believe that to be true anymore. So if the cost of not being able to adopt this feature is only local to a crate, it doesn't matter that much.

That being said, I still think we can do better in the future.

Part of the problem is not require an MSRV bump is very out of the ordinary with the only cases I can think of being package.rust-version and lints.

I can throw any random key in my Cargo.toml and will only get a warning that it wasn't used. That is, I believe, what cargo should have continued doing with the public key. Cargo attempted to approach stability issues the same way the language does, by gating features with hard errors, when in fact I believe it needs a different approach.

Your RFC gets this right, by phasing in enforcement as a warning (with opt-in, even) and turning it into a hard error over an edition. I can think of other approaches that use this "two factor" opt-in idea to get away without waiting for an edition.

We could consider dropping the nightly requirement now so we at least have a couple release lead time on this problem (with the risk of stabilzation being even further out than that with the state of the compiler side).

Yes, I think we should do that in any case.

I do think this conversation represents a deeper problem: a push towards stagnation due to MSRV. I'm working on the update for the MSRV resolver RFC that I'm hoping will help lay out a better path for MSRV policies that can help push us out of this stagnation, shifting costs to those that need old versions rather than being old Rust versions being a millstone around the communities neck.

Respectfully, I think we still have to take version skew issues seriously in the design of our package manager. Shifting the cost to the users who want or need the most stability that might help motivate updates in some instances, but in others it will just make Rust a less viable option. Having access to recent versions of ecosystem crates is a security matter.

There is a fundamental tension between stability and stagnation. How confident can you be that your MSRV RFC will modify the revealed preferences of ecosystem libraries and their users? There will always be a cost to updating unless we take a very hard line stance on backward compatibility, which historically we have not been willing to do (and I don't think we should).

epage commented 8 months ago

I can throw any random key in my Cargo.toml and will only get a warning that it wasn't used. That is, I believe, what cargo should have continued doing with the public key. Cargo attempted to approach stability issues the same way the language does, by gating features with hard errors, when in fact I believe it needs a different approach.

Its not as simple as "it was ignored before so it should be fine to not error while unstable". Depending on how the key changes behavior, ignoring it would run counter to the users intention. While we can't go back in time to ensure the key was never allowed, the unstable period offers us the chance to ensure users will get a hard error, telling them that the feature they are trying to use is unsupported and cargo cannot fulfill their intent. This also discourages users mixing nightly and stable code where an upgrade of stable can break them because the stabilized form of the feature is different.

To balance this out, I was the one who advocated for [lints] to not error on stable (and I went out of my way in the implementation to deal with nightly users in case the format changed). I also was originally advocating for it here. We should be looking for opportunities where it works to not error while its unstable. Its just not a blanket thing we should do.

epage commented 8 months ago

We discussed the blocking feature gate in today's cargo meeting and I've posted rust-lang/cargo#13307 to clarify the documentation to encourage more non-blocking gates and I've opened rust-lang/cargo#13308 to change our feature gate here.

tmandry commented 7 months ago

Thanks @epage, I really appreciate your receptiveness here!

weihanglo commented 6 months ago

See https://github.com/rust-lang/rust/pull/121710#issuecomment-2016344080.

The non-blocking gate -Zpublic-dependency turns out blocking bootstrapping rustc, with a flood of exported_private_dependencies lint errors. That's because cargo-features = ["public-dependency"] is scoped to a certain crate, but with -Zpublic-dependency the entire dependency graph starts passing --extern priv:<crate> unconditionally.

epage commented 6 months ago

Probably worth keeping both opt-ins.

traviscross commented 3 months ago

@rustbot labels +E-help-wanted

If this item is going to make it into Rust 2024 (and it's OK if it doesn't), we're going to need some help to check off the remaining items above.

As @davidtwco describes:

I think the compiler's implementation needs to change quite a bit. We currently compute the private-ness of a dependency and then that is used by the lint whenever we see an item from that dependency, regardless of the path you access the item from. I think that's confusing but also if an intermediate dependency changes which of its dependencies it re-exports from then something that didn't lint might start linting and all sorts of things like that. However, the implementation of lint uses our visibility infrastructure, at which point it's far too late to be able to know which path you took to reference an item, that information isn't really available after name resolution. I experimented with trying to stash away some information that I could later use to do this, but nothing felt right.

We'll mark this as E-help-wanted. If you're interested in seeing this happen in time for the edition and pitching in here, please comment on this Zulip thread.

traviscross commented 3 months ago

@rustbot labels -A-edition-2024 +A-maybe-future-edition

Without an owner driving this work forward, this is looking unlikely for this edition, so let's drop that label and mark this as a possibility for a future edition.