Pass ParserOptions to the parser

[ntBre]

Summary

This is part of the preparation for detecting syntax errors in the parser from #16090. As suggested in this comment, I started working on a ParseOptions struct that could be stored in the parser. For this initial refactor, I only made it hold the existing Mode option, but for syntax errors, we will also need it to have a PythonVersion. For that use case, I'm picturing something like a ParseOptions::with_python_version method, so you can extend the current calls to something like

ParseOptions::from(mode).with_python_version(settings.target_version)

But I thought it was worth adding ParseOptions alone without changing any other behavior first.

Most of the diff is just updating call sites taking Mode to take ParseOptions::from(Mode) or those taking PySourceTypes to take ParseOptions::from(PySourceType). The interesting changes are in the new parser/options.rs file and smaller parts of parser/mod.rs and ruff_python_parser/src/lib.rs.

Outdated implementation details for future reference

NOTE: as the details summary says, this does not correspond to the current implementation, which preserves the original signature of parse_unchecked_source and uses a much simpler representation of ParseOptions. This is preserved just for historical purposes.

The ParserOptions implementation is complicated a bit by wanting to preserve the SAFETY comment in parse_unchecked_source:

ruff/crates/ruff_python_parser/src/lib.rs

Lines 289 to 295 in b5cd4f2

	pub fn parse_unchecked_source(source: &str, source_type: PySourceType) -> Parsed<ModModule> {
	// SAFETY: Safe because `PySourceType` always parses to a `ModModule`
	Parser::new(source, source_type.as_mode())
	.parse()
	.try_into_module()
	.unwrap()
	}

The only way I could see to enforce this while passing a ParserOptions instead of a PySourceType was by adding a generic type state to ParserOptions, so that parse_unchecked_source would take a ParserOptions<KnownSource> constructed by ParserOptions::from_source_type, as opposed to the ParserOptions<UnknownSource> produced by ParserOptions::from_mode.

On top of that, I didn't want to propagate the generics from ParserOptions to all uses of Parser, so there's another indirection through the AsParserOptions trait, which is used by Parser to hold a Box<dyn AsParserOptions> instead of a ParserOptions<S: SourceType>.

Finally, other public functions in the parser crate could be updated to take ParserOptions if we want, but parse_unchecked_source is the variant that ruff and red-knot call where I want to integrate the syntax error checks, so it was my main priority.

Test Plan

Existing tests, this should not change any behavior.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

[dhruvmanila]

The only way I could see to enforce this while passing a ParserOptions instead of a PySourceType was by adding a generic type state to ParserOptions, so that parse_unchecked_source would take a ParserOptions<KnownSource> constructed by ParserOptions::from_source_type, as opposed to the ParserOptions<UnknownSource> produced by ParserOptions::from_mode.

What's the motivation for changing the function signature for parse_unchecked_source? I think we should keep it as is and accept PySourceType which is then internally converted into a ParserOptions. This way we can maintain the invariant that PySourceType always parses into a ModModule. This will then avoid the need for KnownSource.

---

I've some additional thoughts that are related to this:

• I'm wondering if we should abstract away the Mode parameter i.e., replace mode: Mode with options: ParserOptions in parse and parse_unchecked functions
• We could possibly remove Mode::Ipython and use PySourceType::Ipynb instead because the ParserOptions will contain it and it seems a bit redundant that both Mode and PySourceType, which the parser and lexer have access to, contains an indicator that we're in a notebook context.
• I think we should also pass the options to the Lexer. I don't think the TokenSource needs any info from it but even if it did, it can just ask the lexer.

Curious to hear @MichaReiser thoughts on this.

[MichaReiser]

I agree with @dhruvmanila that I wouldn't make ParserOptions generic or add ParserOptions to parse_unchecked_source. It unnecessarily complicates things internally.

We could consider adding a ParseSourceOptions struct which is ParserOptions with the source type prop omitted, but I'm not sure if it's worth exposing the options on parse_unchecked_source. Instead, advanced use cases should use pare_unchecked and then map/unwrap on the call site.

I think it could make sense to expose the ParserOptions to the Lexer, or at least a subset of the options but I think this is something we can tackle separately.

[AlexWaygood]

I'll leave this one to Micha/Dhruv :-)

[ntBre]

The motivation for changing the signature of parse_unchecked_source in particular is that it's called by ruff and by red-knot where I wanted to inject the Python version for syntax error detection. I wanted to avoid moving the unwrap call and accompanying SAFETY comments to those call sites, but that could certainly be preferable to the complication of the generics here.

And the Box<dyn AsParserOptions> follows from that to erase the generic on ParserOptions to avoid making every Parser a Parser<T>. That would obviously go away if ParserOptions were not generic.

[ntBre]

I've reverted the generic stuff and put source_type back in parse_unchecked_source. I can just change ruff and red-knot to call and unwrap parse_unchecked when I actually integrate the syntax error checks.

I also changed parse and parse_unchecked to take ParserOptions instead of Mode, as suggested. That will work nicely with the point above.

We could possibly remove Mode::Ipython and use PySourceType::Ipynb instead because the ParserOptions will contain it and it seems a bit redundant that both Mode and PySourceType, which the parser and
lexer have access to, contains an indicator that we're in a notebook context.

I looked into this briefly, but I don't think the lexer currently has access to a PySourceType, so this might make more sense to combine with passing other options to the lexer too.

[MichaReiser]

Sweet. I recommend giving @dhruvmanila some time to review the PR too.

[MichaReiser]

Nit: We could implement from_source_type (or From<SourceType> for ParserOptions)

[ntBre]

Okay, I was thinking about that too. Should I implement From<Mode> for ParseOptions too? I guess I was thinking we might have multiple from_* methods, but I'm realizing now that they won't actually conflict with each other.

[ntBre]

This gives me more to put in the separate options.rs file anyway. I thought it looked a bit sparse before 😄

[MichaReiser]

I have a love-hate relationship with From and I've been very inconsistent in the past with having explicit from_ methods vs From implementations...

I do like that from_mode is explicit but it also is a bit verbose... 🤷

[ntBre]

I know what you mean. I already switched to From immediately after commenting, but I'm happy to go back to the explicit versions if you and/or Dhruv prefer. I think I tend toward from_ methods usually, so I can really go either way.

[MichaReiser]

I defer to you and @dhruvmanila

[dhruvmanila]

I think From is fine. In the future, when there are more options and if it turns out a bit difficult to read, we could switch to from_ methods. Another option would be to use a builder pattern with the default impl so that it reads as ParseOptions::default().with_mode(mode).with_python_version(version).

[dhruvmanila]

LGTM! I'd recommend updating the PR description to reflect the current state of the PR for future readers. We can keep the "Details" content as a collapsed section for historical purposes.

[ntBre]

I forgot to update the ParserOptions in the title 🤦 but I did update the description!