Skip to content

feat(pkgid-spec): Builtin package spec parsing and formatting #17539

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.

Sign up for GitHub

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

Merged
weihanglo merged 27 commits into from
Oct 6, 2026
Merged

feat(pkgid-spec): Builtin package spec parsing and formatting #17539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
ba645e4
refactor(source-id): Update the fixed builtin URL
adamgemmell Sep 29, 2026
849b350
refactor(pkgid-spec): Update `parse_spec` return type to be more honest
adamgemmell Sep 29, 2026
b61901d
test(pkgid-spec): Positive test for builtin pkgid spec parsing
adamgemmell Sep 28, 2026
dc53430
feat(pkgid-spec): Successful parsing of builtin pkgid specs
adamgemmell Aug 26, 2026
3417567
test(pkgid-spec): Parse builtins with a "builtin+" kind
adamgemmell Sep 28, 2026
9d326b3
fix(pkgid-spec): Parse builtin urls with protocol
adamgemmell Sep 28, 2026
5eb6718
test(pkgid-spec): Builtin URL with no fragment
adamgemmell Sep 29, 2026
8068859
fix(pkgid-spec): Return error when builtin URL is missing a fragment
adamgemmell Sep 29, 2026
89043ff
test(pkgid-spec): Accept version specifier on builtins
adamgemmell Sep 29, 2026
ce409b7
fix(pkgid-spec): Parse and print version for builtin pkgid
adamgemmell Sep 29, 2026
74bf558
test(pkgid-spec): Test for invalid builtin version
adamgemmell Sep 29, 2026
81bc0d1
fix(pkgid-spec): Reject builtin versions that don't match 0.0.0
adamgemmell Sep 29, 2026
41d2142
test(pkgid-spec): Add regression test for partial version
adamgemmell Sep 29, 2026
64ed9ad
test(pkgid-spec): Check error message on empty fragment
adamgemmell Sep 29, 2026
e8971d4
fix(pkgid-spec): Return appropriate error message for empty fragment
adamgemmell Sep 29, 2026
39b8622
test(pkgid-spec): Add regression test for missing name in fragment
adamgemmell Sep 29, 2026
cbde4f3
test(pkgid-spec): Test invalid hostname
adamgemmell Sep 29, 2026
d371fb8
fix(pkgid-spec): Assert that hostname for builtin scheme should be `.`
adamgemmell Sep 29, 2026
022da65
test(pkgid-spec): Test for path in builtin URL
adamgemmell Sep 29, 2026
86298bc
fix(pkgid-spec): Reject paths for builtin packages
adamgemmell Sep 29, 2026
6cdd36e
test(pkgid-spec): Test for builtin scheme when kind is builtin
adamgemmell Sep 29, 2026
c321769
fix(pkgid-spec): Error on invalid protocol for builtin scheme
adamgemmell Sep 29, 2026
6f2423e
test(pkgid-spec): Test for ?query with builtin
adamgemmell Sep 30, 2026
2017a36
fix(pkgid-spec): Reject query string for builtin scheme
adamgemmell Sep 30, 2026
5146925
test(pkgid-spec): Test for query with builtin+ kind
adamgemmell Sep 30, 2026
53badd8
fix(pkgid-spec): Reject query string with builtin+ kind
adamgemmell Sep 30, 2026
4289ff0
test(pkgid-spec): Add doctest showing package id spec examples
adamgemmell Oct 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 150 additions & 9 deletions crates/cargo-util-schemas/src/core/package_id_spec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ impl PackageIdSpec {
/// "https://github.com/rust-lang/crates.io-index#[email protected]",
/// "ssh://[email protected]/rust-lang/foo.git#[email protected]",
/// "file:///path/to/my/project/foo",
/// "file:///path/to/my/project/foo#1.1.8"
/// "file:///path/to/my/project/foo#1.1.8",
/// "builtin://.#core"
/// ];
/// for spec in specs {
/// assert!(PackageIdSpec::parse(spec).is_ok());
Expand All @@ -94,7 +95,10 @@ impl PackageIdSpec {
.into());
}
}
let (name, version) = parse_spec(spec)?.unwrap_or_else(|| (spec.to_owned(), None));
let (name, version) = match parse_spec(spec)? {
Some((name, ver)) => (name, Some(ver)),
None => (spec.to_owned(), None),
};
PackageName::new(&name)?;
Ok(PackageIdSpec {
name: String::from(name),
Expand Down Expand Up @@ -140,24 +144,79 @@ impl PackageIdSpec {
kind = Some(SourceKind::Path);
url = strip_url_protocol(&url)?;
}
"builtin" => {
if url.query().is_some() {
return Err(ErrorKind::UnexpectedQueryString(url).into());
}
if scheme != "builtin" {
return Err(ErrorKind::UnsupportedBuiltinScheme(scheme.into()).into());
}
kind = Some(SourceKind::Builtin);
url = strip_url_protocol(&url)?;
}
kind => return Err(ErrorKind::UnsupportedProtocol(kind.into()).into()),
}
} else {
} else if url.scheme() == "builtin" {
if url.query().is_some() {
return Err(ErrorKind::UnexpectedQueryString(url).into());
}
kind = Some(SourceKind::Builtin)
} else if url.query().is_some() {
return Err(ErrorKind::UnexpectedQueryString(url).into());
}

let frag = url.fragment().map(|s| s.to_owned());
url.set_fragment(None);

if kind == Some(SourceKind::Builtin) {
// Builtins are different in that they require a fragment and cannot have path segments,
// so handle them early
if url.host_str() != Some(".") {
return Err(ErrorKind::InvalidHostname.into());
}
if url.path() != "" {
return Err(ErrorKind::InvalidPath.into());
}
let Some(frag) = frag else {
return Err(ErrorKind::FragmentRequired.into());
};

if frag.is_empty() {
return Err(ErrorKind::EmptyFragment.into());
}
let name = match parse_spec(&frag)? {
Some((name, ver)) => {
if !ver.matches(&Version::new(0, 0, 0)) {
return Err(ErrorKind::InvalidVersion(ver.to_string()).into());
}
name
}
None => frag,
};
Comment on lines +171 to +195

@weihanglo weihanglo Oct 6, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not a blocker: this is a pretty special case that we probably want to have a formal spec grammar doc for it.
(Could be added to the unstable.md doc section first)

View changes since the review

@adamgemmell adamgemmell Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've mentioned this on the tracking issue, and can add a line to the builtin-dependencies unstable feature about it. Something like this?

The Package ID specification can refer to builtin packages using the form builtin://.#<name>. For example, builtin://.#core identifies the builtin core package, distinguishing it from packages named core from other sources.

@weihanglo weihanglo Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sounds good!


PackageName::new(&name)?;
let version = PartialVersion {
major: 0,
minor: Some(0),
patch: Some(0),
pre: None,
build: None,
};
return Ok(PackageIdSpec {
name,
version: Some(version),
url: Some(url),
kind,
});
}

let (name, version) = {
let Some(path_name) = url.path_segments().and_then(|mut p| p.next_back()) else {
return Err(ErrorKind::MissingUrlPath(url).into());
};
match frag {
Some(fragment) => match parse_spec(&fragment)? {
Some((name, ver)) => (name, ver),
Some((name, ver)) => (name, Some(ver)),
None => {
let Some(f) = fragment.chars().next() else {
return Err(PackageIdSpecError(ErrorKind::EmptyFragment));
Expand Down Expand Up @@ -213,7 +272,7 @@ impl PackageIdSpec {
}
}

fn parse_spec(spec: &str) -> Result<Option<(String, Option<PartialVersion>)>> {
fn parse_spec(spec: &str) -> Result<Option<(String, PartialVersion)>> {
let Some((name, ver)) = spec
.rsplit_once('@')
.or_else(|| spec.rsplit_once(':').filter(|(n, _)| !n.ends_with(':')))
Expand All @@ -222,7 +281,7 @@ fn parse_spec(spec: &str) -> Result<Option<(String, Option<PartialVersion>)>> {
};
let name = name.to_owned();
let ver = ver.parse::<PartialVersion>()?;
Ok(Some((name, Some(ver))))
Ok(Some((name, ver)))
}

fn strip_url_protocol(url: &Url) -> Result<Url> {
Expand All @@ -240,7 +299,7 @@ impl fmt::Display for PackageIdSpec {
let mut printed_name = false;
match self.url {
Some(ref url) => {
if let Some(protocol) = self.kind.as_ref().and_then(|k| k.protocol()) {
if let Some(protocol) = self.kind.as_ref().and_then(SourceKind::protocol) {
write!(f, "{protocol}+")?;
}
write!(f, "{}", url)?;
Expand All @@ -249,7 +308,9 @@ impl fmt::Display for PackageIdSpec {
write!(f, "?{}", pretty)?;
}
}
if url.path_segments().unwrap().next_back().unwrap() != &*self.name {
if self.kind() == Some(&SourceKind::Builtin)
|| url.path_segments().unwrap().next_back().unwrap() != &*self.name
{
printed_name = true;
write!(f, "#{}", self.name)?;
}
Expand All @@ -259,7 +320,9 @@ impl fmt::Display for PackageIdSpec {
write!(f, "{}", self.name)?;
}
}
if let Some(ref v) = self.version {
if let Some(ref v) = self.version
&& self.kind != Some(SourceKind::Builtin)
{
write!(f, "{}{}", if printed_name { "@" } else { "#" }, v)?;
}
Ok(())
Expand Down Expand Up @@ -333,6 +396,23 @@ enum ErrorKind {
#[error("pkgid url cannot have an empty fragment")]
EmptyFragment,

#[error("builtin package ID specifications must specify the package name in the fragment")]
FragmentRequired,

#[error("version `{0}` is invalid for builtin packages, which are unversioned (or `0.0.0`)")]
InvalidVersion(String),

#[error("only `.` is permitted as a hostname for builtin package ID specifications")]
InvalidHostname,

#[error("only empty paths are permitted for builtin package ID specifications")]
InvalidPath,

#[error(
"`builtin+{0}` is unsupported - only the `builtin` protocol is supported for `builtin` kinds"
)]
UnsupportedBuiltinScheme(String),

#[error(transparent)]
NameValidation(#[from] crate::restricted_names::NameValidationError),

Expand Down Expand Up @@ -738,6 +818,46 @@ mod tests {
},
"path+file:///path/to/my/project/foo#foo::[email protected]",
);
ok(
"builtin://.#core",
PackageIdSpec {
name: String::from("core"),
version: Some("0.0.0".parse().unwrap()),
url: Some(Url::parse("builtin://.").unwrap()),
kind: Some(SourceKind::Builtin),
},
"builtin://.#core",
);
ok(
"builtin+builtin://.#core",
PackageIdSpec {
name: String::from("core"),
version: Some("0.0.0".parse().unwrap()),
url: Some(Url::parse("builtin://.").unwrap()),
kind: Some(SourceKind::Builtin),
},
"builtin://.#core",
);
ok(
"builtin+builtin://.#[email protected]",

@epage epage Sep 30, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we accept builtin+builtin?

View changes since the review

@adamgemmell adamgemmell Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the answer here is the same as part of my answer for versions:

I'm accepting them for consistency with existing pkgid specifications. I've tried to avoid limiting the documented grammar where possible (though the parsing already only accepts a subset of what the grammar specifies).

However I'm less sure about this one. I'm also going to modify the formatter not to emit the kind+ if present.

@adamgemmell adamgemmell Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm also going to modify the formatter not to emit the kind+

Doing so required inferring the kind from the scheme in order to not break roundtripping, which I think is ok as it's now the authoritative source on whether the pkgid spec is a builtin.

@epage epage Sep 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we support +, I'm unsure about making the lack of it the canonical form.

An alternative framing is "we start with the minimum supported syntax and then add as we feel the need" so we stabilize the least. Granted, this affects canonical forms since we need to be stable on those.

Eh, let's put this in the tracing issue and figure it out later.

@epage epage Sep 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To clarify: both cases should be in the tracking issue

@adamgemmell adamgemmell Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added both cases to the tracking issue #16960 (comment) so we can address this before stabilising anything.

@epage epage Sep 30, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we accept versions?

View changes since the review

@adamgemmell adamgemmell Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm accepting them for consistency with existing pkgid specifications. I've tried to avoid limiting the documented grammar where possible (though the parsing already only accepts a subset of what the grammar specifies).

We need to publish these versions in lockfiles at least. I'm not sure who uses the full package id spec format but I imagine automated tooling may want the version to be accepted.

I'm going to change the PR to not emit the version however since it isn't needed at all. Personally I'd like to pretend that builtin packages are unversioned where possible given that we don't accept the version key in the manifest.

@adamgemmell adamgemmell Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm going to change the PR to not emit the version

Doing so required inferring the version when it isn't provided in order to not break roundtripping.

@adamgemmell adamgemmell Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Another thought on versions, we can't reject them when doing the simpler [email protected] package ID form (as it may also match path/git dependencies). It would be odd for that to be accepted, but the strictly more specific builtin://.#[email protected] form to be rejected.

PackageIdSpec {
name: String::from("core"),
version: Some("0.0.0".parse().unwrap()),
url: Some(Url::parse("builtin://.").unwrap()),
kind: Some(SourceKind::Builtin),
},
"builtin://.#core",
);
ok(
"builtin+builtin://.#core@0",
PackageIdSpec {
name: String::from("core"),
version: Some("0.0.0".parse().unwrap()),
url: Some(Url::parse("builtin://.").unwrap()),
kind: Some(SourceKind::Builtin),
},
"builtin://.#core",
);
}

#[test]
Expand Down Expand Up @@ -785,5 +905,26 @@ mod tests {
ErrorKind::InvalidPkgIdUrl(_)
);
err!("git+https://", ErrorKind::InvalidPkgIdUrl(_));
err!("builtin://.", ErrorKind::FragmentRequired);
err!(
"builtin+builtin://.#[email protected]",
ErrorKind::InvalidVersion(_)
);
err!("builtin://.#", ErrorKind::EmptyFragment);
err!("builtin://.#0.0.0", ErrorKind::NameValidation(_));
err!("builtin://wrong#core", ErrorKind::InvalidHostname);
err!("builtin://./invalid/path#core", ErrorKind::InvalidPath);
err!(
"builtin+invalid://.",
ErrorKind::UnsupportedBuiltinScheme(_)
);
err!(
"builtin://.?query=test#core",
ErrorKind::UnexpectedQueryString(_)
);
err!(
"builtin+builtin://.?query=test#core",
ErrorKind::UnexpectedQueryString(_)
);
}
}
3 changes: 2 additions & 1 deletion crates/cargo-util-schemas/src/core/source_kind.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ impl SourceKind {
SourceKind::SparseRegistry => None,
SourceKind::LocalRegistry => Some("local-registry"),
SourceKind::Directory => Some("directory"),
SourceKind::Builtin => Some("builtin"),
// The protocol is not required for builtins so we don't emit it
SourceKind::Builtin => None,

@adamgemmell adamgemmell Sep 30, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that doing this breaks roundtripping for sourceid URLs. I expect to address this when addressing builtins in lockfiles, if not sooner.

View changes since the review

@epage epage Sep 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How do you plan to address it and why is addressing it being deferred when it sounds like it is relevant to this PR?

@adamgemmell adamgemmell Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Roundtripping SourceId URLs is required for encoding and parsing lockfiles, so it won't be something that's forgotten later on.

It's not in this PR as my understanding is that no unit tests exercise that path yet as far as I'm aware and I don't think it's possible to write a user-facing test for it yet.

My view is that it's a separate part of Cargo and in the RFC has a different expected form than the package id specs:

[[package]]
name = "std"
version = "0.0.0"
source = "builtin"

I could cover it in this PR if you like, or we could defer merging this PR until a followup handling it is available.

}
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/workspace/source_id.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ impl SourceId {

/// Creates a `SourceId` for the builtin packages in the configured toolchain.
pub fn for_builtin() -> CargoResult<SourceId> {
let url = "builtin://".into_url()?;
let url = "builtin://.".into_url()?;
SourceId::new(SourceKind::Builtin, url, None)
}

Expand Down Expand Up @@ -856,8 +856,8 @@ mod tests {
assert_data_eq!(short_hash(&source_id), str!["199e591d94239206"].raw());

let source_id = SourceId::for_builtin().unwrap();
assert_data_eq!(gen_hash(source_id), str!["8471877086995577839"].raw());
assert_data_eq!(short_hash(&source_id), str!["ef7b241f53279275"].raw());
assert_data_eq!(gen_hash(source_id), str!["5062137541645472653"].raw());
assert_data_eq!(short_hash(&source_id), str!["8d93f21546534046"].raw());

let path = &ws_root.join("crate");
let source_id = SourceId::for_local_registry(path).unwrap();
Expand Down
Loading