Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Refactor settings & improve dx (#19766)
This PR aims to:
- Expose Settings for ScalaCLI so it does not need to instantiate
Context and can discover settings & categories. (1)
- Get rid of mutable state within Settings, so we can create an
immutable DSL for implementing options like `-W` ("-Wall") (2)
- Standardise parsing & structure of the settings. (3)
- Refactor the settings to prevent problems in the future. (4)
## Goals
Here is the additional context for each of the goals:
### (1) Exposing Settings
ScalaSettings was a class that was instantiated for each compiler
context. Each time it was the same - the state was (almost) separated
from it and put in `SettingsState`. Due to it being a class, tooling
needed an artificial compilation context to access the fields. Even
after acquiring this `ScalaSettings` instance, the settings did not
expose API with the required access paths: (provided by ScalaCLI team)
- List of all keys accepted by the Compiler
- List of categories prefixes (-V, -W, etc.)
- List of keys not requiring source inputs
- List of alises for keys / access to keys via aliases
- Way to determine how a setting accepts options (after `:` or space?)
- Way to see if settings are a flag.
- Way to see if a setting can be provided multiple times.
Additionally, the ScalaSettings were the same each time and represented
a static structure - it would lower complexity & limit potential
problems if made an immutable object instead.
### (2) Mutable state
Settings still had a piece of mutable state inside of them - it was used
to store a flag denoting whether the setting was set repeatedly. It
could be stored in SettingsState instead, allowing an immutable DSL on
Settings.
### (3) Standarization
Settings contain some behaviors that may be hard to get right without
preexisting knowledge. They do not have a standard enforced upon them.
That causes two problems:
- It may be misleading to users & worsen the DX
- It was reported as a cause of problems in integration with ScalaCLI
and may cause similar problems in the future for other tools
These problems include:
- Some options support only passing parameters after a semicolon, and
some allow passing them after space: Strings/Ints allow after space,
VersionTags/Files/Lists do not, etc, and there's no way to know which is
which without trying it out
- Some have `--` aliases, some don't
### (4) Refactor
Mutable state in Settings, various ways of handling Setting values (like
`-Ykind-projector`), no validation of settings names/states, no way of
enforcing categories in settings were all problems that caused a handful
of constructs that made it harder to work with settings. I recognized
that it was a good moment to refactor the code & enforce some rules and
patterns.
## Solutions
### (1) Exposing Settings
- Made ScalaSettings an immutable public object (made Setting immutable
alongside)
- Added methods allowing access to required data
- Added categories to Setting and made prefix Optional for
discoverability
The requirement of checking if provided options run help instead of
requiring source can be done by passing now immutable ScalaSettings and
SettingsState to ScalacCommand `isHelpArg`
### (2) Mutable state
`changed` field was moved to SettingsState as a Set instead of storing
it as a mutable field in the Setting. Now `Setting` describes the
structure of settings, and the whole state is represented by
`SettingsState`. The plan is to use this property for a similar goal as
the "post set hooks" in Scala 2 (aside from exposing the API)
### (3) Standarization
- All options can now be passed with either `--` or `-`. There are some
exceptions that come from aliases kept for backward compatibility.
- Setting names are now validated against simple regex
`[a-zA-Z0-9_\\-]*`
- Setting names are now validated so they do not contain `-` at the
beginning and contains category prefix (like `W`)
- All arguments can now be passed after space or `:`. `-Ykind-projector`
is an exception that was kept for backward compatibility. Set of
additional rules was introduced for the settings to make sure that
nothing breaks and compatibility is kept:
- Empty strings are not supported as choice arguments. They were not
supported for any other parameter type, only for `String` parameters
that accept a set of `choice` values. If an empty choice setting was
allowed, passing no value would result in empty value being chosen.
Example: `-Ykind-projector` accepts parameters:
`-Ykind-projector:underscores`. But if it was passed as just
`-Ykind-projector`, then the value would be set to an empty string. This
feature made it hard to keep backward compatibility after making it
possible to pass arguments for every type of setting after a space: One
could write `-Ykind-projector Main.scala` and mistakenly pass
`Main.scala` as an arg for the setting. Luckily, `Ykind-projector` is
the only setting that used this feature and is kept as legacy, no new
settings with this behavior are allowed.
- List settings cannot accept ignore invalid args. Luckily, no setting
was doing that. If possible, an existing command that works but produces
a warning like `-foo Main.scala` (for`-foo` accepting list of string
args) would stop working. String commands did accept passing arguments
after space before, but this is a new behavior for the list settings. If
ignoring invalid args was possible, commands like `-foo Main.scala`
could work unexpectedly.
### (4) Refactor
It's mostly visible in the sources. For example, there is no need to
change `classfileVersion` in two places every time.
### Additional changes
-Xlint was deprecated in favor of -Wshadow, as it was only used for
shadow warnings in the current shape. It's TBD, but from my perspective,
there is no point in keeping some warnings as args for -Xlint, and some
as separate -W settings.- Loading branch information
