Refactor settings & improve dx

[szymon-rd]

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.

[dwijnand]

What's the difference between Some("") and None?

[szymon-rd]

If the prefix of a setting exists, then this setting behaves differently; it is a different type of setting. API users can choose not to provide this prefix, thus not using this feature.

Sole String does not express the property of being optional. String would mean that it either has an empty prefix (so that would include all possible settings or some other feature) or that there is no prefix. Option type makes it clear that the prefix can be not present. It is similar to the conflict of None vsSome(null). Ideally, we'd like an Option[NonEmptyString] there. But now, Some("") just is the most general prefix that includes all options.

[dwijnand]

I can't follow you. Can you give 3 reasonable examples, with None, Some(""), and Some("-Whatever")? Might be easier to follow your reasoning.

[odersky]

Generally in the compiler we prefer sentinels over Option. That's the adopted code style. Option can be useful sometimes, but as Dale says we should not reach for it blindly.

[nicolasstucki]

Why do you need this?

[szymon-rd]

I didn't want to have .type everywhere ScalaSettings are stored, I'd rather have it as a type. But it kind of gives a false impression of being instantiable and is not a widely used pattern here. And is not really necessary in the end, we don't need an instance of it as val/field, as it's now immutable. But it's still a draft, a final shape will be there when I convert it to a full PR.

[nicolasstucki]

Now that this is a singleton we could use ScalaSettings instead of settings. Not sure what this val gives us. Same for def settings: ScalaSettings.type = base.settings.

I know that would imply a massive refactoring of all ctx.settings.XYZ.value to ScalaSettings.XYZ.value.

[szymon-rd]

I know that would imply a massive refactoring of all ctx.settings.XYZ.value to ScalaSettings.XYZ.value.

That's the reason. There is no cost to keeping it a setting right now, and the PR is getting quite big (a change in params parsing is coming here too). This refactor will be done in next PR, so we don't block the ScalaCLI integration with it.

[lrytz]

@szymon-rd could you add a PR description (what changes for users, in the settings API, what is being fixed, etc)?

[szymon-rd]

@lrytz I will write detailed description before making it ready for review

[lrytz]

Thanks, what you added is already really helpful!

[dwijnand]

Nice description, @szymon-rd !

[szymon-rd]

TODO: Run full open CB on this

[szymon-rd]

Some projects in CB failed. After investigation, I found the cause: Some macro libraries, especially Enumeratum, use ScalaSettings directly from the compiler. I think we should be able to have a ScalaSettings trait & companion object implementing it and have it fix it.

[nicolasstucki]

We might transform this into a Map[Int, ...]. This way we would simplify classfileVersionMap.keysIterator.map(_.toInt).min to classfileVersionMap.keysIterator.min.

[nicolasstucki]

I would add some documentation to all these settings.

[nicolasstucki]

This should probably be an enumeration. It seems too easy to pass an arbitrary string to the category of BooleanSetting (or similar method).

[nicolasstucki]

This concept deserves its own file.

[nicolasstucki]

Suggested change

	val settingsByCategory: Map[String, List[Setting[_]]] =
	private val settingsByCategory: Map[String, List[Setting[_]]] =

[szymon-rd]

I kept it intentionally as public, It may be useful in cli tooling

[nicolasstucki]

The order of keys is not deterministic, but it should probably be deterministic.

[nicolasstucki]

This would accept the empty name and names that start with _ or -. Maybe the regex should be [a-zA-Z][a-zA-Z0-9_\\-]*.

[nicolasstucki]

This would also accept a name that starts with X, Y, V, .... Maybe we the regex can be [a-z][a-zA-Z0-9_\\-]*.

[szymon-rd]

I kept the prefix in the name to avoid too much magic (people will likely search for e.g. "Wunused" in the ScalaSettings.scala file)

[szymon-rd]

as for the first comment, aliases are also validated, and they are kept as they were for now for back compat. They contain one or two - in prefix. In general, whether a setting starts with - or has an appropriate prefix is validated in the Setting itself. I will put what I can in the Setting class.

[nicolasstucki]

It might be simpler to pass the name as is and validate it in Setting.

[szymon-rd]

It prepends the "-" for backward compatibility, so the names don't change after this PR. But we don't pass "-" anymore in the config. Little magic, but seems to be optimal out of all possibilities. I will put what I can in the Setting class.

[szymon-rd]

Please do not merge yet, I am still validating community builds.

[szymon-rd]

There seems to be a last problem: Some project in CB B failed with
Error: missing argument for option -Xmigration
It seems to be the last problematic option

[SethTisue]

thorough writeup in PR description much appreciated 👍