refactor: configuration, settings and options

[Sec-ant]

Summary

A preliminary attempt to address #3297, #3310 (comment) etc.

• Use Option<T> in JsParserSettings
• Use a consistent naming style, and improve the doc comments for sustainable maintenance.
• Retain a single source of truth of defaults.
• Make sure that caching works as expected.
• Try removing the Partial macro.

Test Plan

CI passes. No performance degradation.

[github-actions]

Parser conformance results on

js/262

Test result	main count	This PR count	Difference
Total	48353	48353	0
Passed	47156	47156	0
Failed	1197	1197	0
Panics	0	0	0
Coverage	97.52%	97.52%	0.00%

jsx/babel

Test result	main count	This PR count	Difference
Total	40	40	0
Passed	37	37	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.50%	92.50%	0.00%

symbols/microsoft

Test result	main count	This PR count	Difference
Total	6574	6574	0
Passed	2213	2213	0
Failed	4361	4361	0
Panics	0	0	0
Coverage	33.66%	33.66%	0.00%

ts/babel

Test result	main count	This PR count	Difference
Total	670	670	0
Passed	598	598	0
Failed	72	72	0
Panics	0	0	0
Coverage	89.25%	89.25%	0.00%

ts/microsoft

Test result	main count	This PR count	Difference
Total	18302	18302	0
Passed	14029	14029	0
Failed	4273	4273	0
Panics	0	0	0
Coverage	76.65%	76.65%	0.00%

[codspeed-hq]

CodSpeed Performance Report

Merging #3317 will not alter performance

_{Comparing refactor/configuration-settings-options (b957d12) with main (84384cf)}

Summary

✅ 95 untouched benchmarks

[Sec-ant]

Most of the other diffs are caused by variable renaming. This file is the starting point.

[dyc3]

Looks good to me so far.

[Conaclos]

Try removing the Partial macro.

Could you explain the reasons for removing this macro and what you are using instead?

[Sec-ant]

Try removing the Partial macro.

Could you explain the reasons for removing this macro and what you are using instead?

The reason is that we almost always want to wrap options as Option<T> in our internal structure because when 1. merging configurations 2. applying language-specific options to their language-agnositc counterparts 3. applying overrides to base options, we need the None state to indicate that one option will not overwrite the other.

Currently, we have some bugs when converting between concrete structs and partial structs, which may accidentally convert a None to a Some and overwrite things unexpectedly. I also find this a source of confusion.

What I'm using instead is forcing Option<T> for every option, and I plan to postpone the unwrapping/resolution to the phase when a concrete value is actually needed, e.g. when resolving the ParseOptions / FormatOptions, when printing the biome rage messages, etc.

I'm also experimenting with a model where each option carries its default value in its type directly, so that whenever we need to get a concrete value of an option, we just need .unwrap_or_default().

[Conaclos]

Thanks for the rationales.
I agree with your points. Overall, I think this will reduce the complexity of the configuration.

[Sec-ant]

I have some questions regarding how we handle ignored and included. There might be some problems in our current code.

Based on my understanding of this feature, I summarized the logic as the table below. Is it ok that I refactor the code to use 👇 to determine whether a path should be included or ignored?

Ignored list \ Included list	Some(Patterns) & Matched	Some(Patterns) & Unmatched	None
Some(Patterns) & Matched	Path is ignored	Path is ignored	Path is ignored
Some(Patterns) & Unmatched	Path is included	Path is ignored	Path is included
None	Path is included	Path is ignored	Path is included

[ematipico]

@Sec-ant yeah that's about right.

Empty lists should not run any matching.

The golden rule is that ignored has precedence over include, always.

If include isn't empty, any path that doesn't match is automatically ignored.

[Conaclos]

Based on my understanding of this feature, I summarized the logic as the table below. Is it ok that I refactor the code to use 👇 to determine whether a path should be included or ignored?

Everything looks good to me. I changed this in January / February to have a more consistent behavior. As Ema said ignore always wins on include. If include is None, then all files are included. If it is not None, then only matched files are considered. Then ignore is applied.

What looks wrong to you?

If I were to design the configuration now, I think I would just give a single field (e.g. files or ignores) that supports negative globs and apply globs in order. This might make the system more powerful and more intuitive.

[Sec-ant]

Everything looks good to me. I changed this in January / February to have a more consistent behavior. As Ema said ignore always wins on include. If include is None, then all files are included. If it is not None, then only matched files are considered. Then ignore is applied.

What looks wrong to you?

Oh, this is exactly how I see it, so nothing looks wrong to me :).

The only thing I'd like to keep you informed before doing it is that, now that we decide to always use Option<T> in this PR to refactor the options, I'd like to keep a consistent mental model across the codebase. Currently, it seems that we use Vec::empty to indicate the list is unset, I'd like to suggest that we use None for this purpose, and regard any Some values as being explicitly set, even if it's an empty vector. I'll also make sure this will not change the default behavior.

[Sec-ant]

And FWIW, I'll keep working on this PR in the following days but I cannot gaurantee a specific timeframe, so this shouldn't be a blocker for any foreseeable release plans, just in case.