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

Pass derivation details through from Rust. #21631

Merged

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Nov 12, 2024

This allows us to specify which config file each value came from.

We intern the details strings, as they are very repetitive. This requires
tying the lifetime of various data structures to that of the GIL.

This allows us to specify which config file each
value came from.

We intern the details strings, since they are very
repetitive from a small set of possible strings.
This requires explicit lifetimes in more places,
but hey, that's what Rust is for.
@benjyw benjyw added the category:internal CI, fixes for not-yet-released features, etc. label Nov 12, 2024
@benjyw benjyw requested a review from huonw November 12, 2024 05:02
fn source_to_details(source: &Source) -> Option<&str> {
match source {
Source::Default => None,
Source::Config { ordinal: _, path } => Some(path),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dispensed with the prefix "from" in these, since it adds no information, and in the case of config file paths it would require storing a "from " string somewhere with an appropriate lifetime, and there is no obvious place for that (other than on the Source itself, but that seems silly).

// intersperse is provided by itertools::Itertools, but is also in the Rust nightly
// as an experimental feature of standard Iterator. If/when that becomes standard we
// can use it, but for now we must squelch the name collision.
Some(PyString::intern_bound(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do end up allocating a new Rust String in this case, but it's an uncommon case so that seems fine (and the Python-side interning will still work).

// In ascending rank order, so the last value is the final value of the option.
type OptionValueDerivation<T> = Vec<(T, Vec<isize>)>;
// The derivation of the option value, as a vec of (value, rank, details string) tuples.
type OptionValueDerivation<'py, T> = Vec<(T, isize, Option<Bound<'py, PyString>>)>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that we may not need that isize after we get rid of the legacy parser. The RankedValue struct on the Python side expects it, but I'm not sure we actually use it.

@benjyw
Copy link
Contributor Author

benjyw commented Nov 12, 2024

Easiest reading order for review:

  • src/python/pants/engine/internals/native_engine.pyi
  • src/rust/engine/src/externs/options.rs
  • src/python/pants/option/native_options.py
  • src/python/pants/help/help_info_extracter.py
  • src/python/pants/help/help_info_extracter_test.py

Thanks!

Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Nice result 👍

@benjyw benjyw merged commit adbe91a into pantsbuild:main Nov 12, 2024
24 checks passed
@benjyw benjyw deleted the use_config_file_path_as_source_details branch November 12, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants