--extern-location to specify where an --extern dependency is defined

[jsgf]

What is this issue?

This is a major change proposal, which means a proposal to make a notable change to the compiler -- one that either alters the architecture of some component, affects a lot of people, or makes a small but noticeable public change (e.g., adding a compiler flag). You can read more about the MCP process on https://forge.rust-lang.org/.

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

MCP Checklist

• MCP filed. Automatically, as a result of filing this issue:
  • The @rust-lang/wg-prioritization group will add this to the triage meeting agenda so folks see it.
  • A Zulip topic in the stream #t-compiler/major changes will be created for this issue.
• MCP seconded. The MCP is "seconded" when a compiler team member or contributor issues the @rustbot second command. This should only be done by someone knowledgable with the area -- before seconding, it may be a good idea to cc other stakeholders as well and get their opinion.
• Final comment period (FCP). Once the MCP is approved, the FCP begins and lasts for 10 days. This is a time for other members to review and raise concerns -- concerns that should block acceptance should be noted as comments on the thread, ideally with a link to Zulip for further discussion.
• MCP Accepted. At the end of the FCP, a compiler team lead will review the comments and discussion and decide whether to accept the MCP.
  • At this point, the major-change-accepted label is added and the issue is closed. You can link to it for future reference.

A note on stability. If your change is proposing a new stable feature, such as a -C flag, then a full team checkoff will be required before the feature can be landed. Often it is better to start with an unstable flag, like a -Z flag, and then move to stabilize as a secondary step.

TL;DR

--extern-location allows a build system to provide better detail about how a unused-crate-dependency warning can be resolved - eg with details on how the Cargo.toml can be updated to remove an unused dependency.

Links and Details

PR rust-lang/rust#72342 introduced the unused-crate-dependency warning, which warns if a crate was specified on the command line with --extern name=path, but that name wasn't referenced in the source. The diagnostic suggests either removing the dependency or adding a dummy reference of the form use name as _;.

The problem is that the "removing dependency" suggestion is vague, as rustc has no idea about how the dependency was specified in the first place, as it is specific to the build system and is out of scope for rustc itself. Also, assuming that removing the dependency is almost always the right course of action, this should be easily automated so that unused dependencies can be mass-removed.

This MCP proposes a --extern-location option which passes information from the build system to rustc, primarily so that rustc can echo it back to the user via a diagnostic message, or back to tooling via json diagnostics.

These options would be paired with --extern options, so a typical use would be --extern foo=path/to/libfoo.rlib --extern-location foo=raw:Cargo.toml:123, where the name foo is the key which joins them.

--extern-location is not used for pathless (sysroot) --extern (eg --extern proc_macro). These are not subject to unused-crate-dependencies warnings and are typically added implicitly anyway.

The goal is to make this build-system independent, rather than just assuming we're always building with Cargo. As such, it needs enough flexibility to be able to express the definition of a dependency in a build-system-generic way.

I have several proposed forms of location:

• file:<path>:<lineno> - reference a specific line in a file
• span:<path>:<startbyte>:<endbyte> - reference a span of bytes in a file
• raw:<string> - uninterpreted text string which is emitted via the diagnostic and json
• json:<jsondata> - json with an arbitrary schema which is emitted only via json diagnostics

file and span both allow rustc to construct Spans internally, so it can actually display the build specification file as part of the diagnostic. This assumes that a dependency can actually be referenced with a single file coordinate.

json requires the schema of json diagnostics to change. I propose a new tool_metadata key which is added to the Diagnostic. By default this is null, but is populated when arbitrary json is passed through. The intent of json is that enough detail can be attached to the diagnostic to pass through to a tool which can act on it - for example, auto-fix the problem, or present a lint or code-review comment.

raw is simple emitted into the rendered stream, but is also put into tool_metadata as a plain json string. This might be better named text.

tool_metadata is also used for an implicit location - if --extern-location isn't specified at all, it is populated with {"name":<crate name>,"path":<crate path>} from the --extern option.

An initial prototype PR is in rust-lang/rust#72603

Notes

I'm primarily testing with the Buck build system, where the most convenient way to express unused dependencies is with a (rule name, dependency) tuple, rather than a file coordinate. As such json is the most interesting form for me.

Discussion with various people on the Cargo team shows that unused-crate-dependencies - and hence --extern-location - are not very useful at the moment, as Cargo doesn't have a way to precisely specify dependencies for specific targets in a Cargo package, and so will result in many spurious warnings. (Cargo would prefer a mechanism to indicate whether a given dependency is unused in all targets rather than specific ones.) Conversation is ongoing about if and how this work can be adapted to Cargo.

Since the general format of --extern-location name=<type>:<data> is extensible, we can remove all the currently unused forms (file, span) and add them back later if needed.

Even though this is a non-Z --extern-location option, the PR still requires -Zunstable-options to be set before its accepted.

Open questions

• What's the best way to represent the tool_metadata in the json diagnostics? Right now its pretty ugly as it appears in every sub-diagnostic. Perhaps it should be a single field in the top-level diagnostic.
• Would it make sense to be able to specify multiple --extern-location arguments per crate, to allow reporting in different forms? For example, if json is always for tools only, then the rendered version of the diagnostic lacks detail. It could make sense to provide both machine and human-readable info in different forms.
• Will all the --extern-location options bloat the command line too much? They're only needed for direct dependencies (not transitive), but they could be very redundant. For example, if every dependency is in the same file, the specifying that file path every time is just a waste of space. I think this is something that can be deferred once we have more practice in experience. We could add a way to back-refer to a previous entry, for example (effectively "foo has the same path as bar").
• There may be other cases where being able to refer to the build dependency specs is useful, but I haven't thought about that much.

Mentors or Reviewers

@petrochenkov reviewed the original unused-crate-dependencies PR (rust-lang/rust#72342) and has given initial comments on rust-lang/rust#72603.

@est31 and @ehuss have been commenting from a Cargo perspective.

@nikomatsakis suggested it should go through the MCP process.

[nagisa]

You mention a Zulip stream, however there's no link to it. Can you please add one?

[jsgf]

That initial part is all boilerplate and the zulip stream was created by automation so it should probably post it here when it does.

But for reference it's https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/--extern-location.20to.20specify.20where.20an.20--ex.20compiler-team.23303

[jsgf]

Hi - what's the next step for this? Has it been discussed?

[nikomatsakis]

@jsgf

There isn't really a central time that we discuss MCPs, it's kind of up to somebody to second the proposal. I think in this case most of the discussion took place on rust-lang/rust#72603 but it does seem to have reached a stopping point. It's a bit hard for me to tell if everyone was happy with the design -- I know that @petrochenkov had some concerns (which they enumerated), so I'd be curious to know whether they felt those were resolved.

The biggest problem is presumably the fact that this doesn't integrate well with the needs of cargo. It'd be good if we at least had an "overall design" that seemed like it could work -- e.g. I could imagine that emitting data about unused dependencies that cargo can collect and combine with the output from other commands or something might be more "forward looking" than having the compiler emit warnings directly. (I saw some notes about json output formats, I'm not sure if there were changes in that direction.)

In any case, I think I would be amenable to merging a change even if we don't really know what cargo will need yet, but with the caveat that we might expect to make changes, potentially large ones, in order to make it work better.

[jsgf]

@nikomatsakis I see - I think most of the discussion happened before the MCP and so all the energy went into the PR. I'll resume discussion in the PR to see if there's anything unresolved in the short term and we can see if there's a medium/longer term solution for Cargo (though I think the current consensus is that Cargo wants to approach the whole problem differently). I'd like to land something in master but I'm fine with keeping it unstable/experimental.

[jsgf]

@nikomatsakis Also I'm a little confused by the MCP process because the Zulip stream has a message from triagebot saying "A new proposal has been announced #303. It will be brought up at the next meeting", so that's what I was waiting for as a next step. If its actually being blocked on being seconded then I can see if I can find a seconder.

[nikomatsakis]

@jsgf ah yeah I guess that is confusing. It is "brought up" (i.e. announced) but we don't usually do detailed discussion there.

[nikomatsakis]

@rustbot second -- as I wrote in this comment, I think we should move forward, though I'm fine with tweaking the design in various ways and not wedded to the details presented here.