Added notes for May 13th 2019

This commit is contained in:
Julien Couvreur 2019-05-14 13:08:43 -07:00 committed by GitHub
parent 563cfe80f0
commit 7a42cf7d94
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23

View file

@ -0,0 +1,96 @@
# C# Language Design Meeting for May 13th, 2019
## Agenda
1. Close on desired rules for warning suppressions and `#nullable` interacting
## Discussion
### Warning suppressions and `#nullable` interacting
The question is what project-level and source-level controls to expose for users to have a pleasant onboarding experience with the nullable feature. We previously leaned towards more flexibility, but want to consider streamlining the design in light of early feedback.
We considered two dimensions of control: (1) whether the types are annotated or not (ie. what is the meaning of `string`?), and (2) whether nullability warnings are produced.
At the project-level at least, all four permutations of those options (disable, enable, annotations-only, and warnings-only) are useful.
| - | Annotations on | Annotations off |
| ------------ | -------------- | --------------- |
| Warnings on | enable | warnings |
| Warnings off | annotations | disable |
Annotations-only are useful for library authors who want to provide annotations for their APIs before they are ready to review their code.
Warnings-only are useful for application consumers who want to benefit from annotations provided by libraries, but are not ready to annotate their own APIs.
Both options offer a stepping stone towards enabling the feature fully.
#### The need for both project-level and source-level controls
Our starting model included the four settings at the project-level (`enable`, `disable`, `annotations` and `warnings`), as well as in source (with `#nullable (enable | disable | restore) [ warnings | annotations ]`).
We observed that, in source, existing warnings cannot be enabled, they can only be suppressed. We considered whether this model could work for nullability warnings too. We don't think so: it creates a significant hurdle to adoption in existing projects. It is much easier to opt-in a few files than to opt-out all but a few files in a project.
For nullability annotations, we considered the converse: should the only way of enabling nullability annotations be in source?
Although this avoids language semantics (the meaning of `string`) being affected by compilation options, it seems undesirable in the long-term. We don't want every file to require some opt-in with `#nullable enable` in new or fully migrated projects.
**Conclusion**: It is useful to control both nullable annotations and warnings at the project-level and in source.
#### Clarifying the interactions
A scenario was brought to light by the BCL team: when updating library, it is useful to temporarily disable nullability warnings at the project-level. This could be solved with the existing `/NoWarn` option, if it were extended to support `/NoWarn:nullable` (where `nullable` is a shorthand for the set of all nullable warnings, including the W-warning/`8600`).
We iterated on this, trying to clarify how `#nullable enable warnings` would interact with `#pragma warning ...`.
Two alternatives emerged:
1. `#nullable enable warnings` and `#pragma warning enable nullable` could be synonyms,
2. `#nullable enable warnings` could be controlling an independent flag that combines with the suppressed/unsuppressed state of a warning to determine whether the warning is produced at a certain position in source.
The first design doesn't offer a very clear mental model when it comes to the order of precedence of various controls (project-level defaults, project-level `/NoWarn`, `#nullable ...` and `#pragma warning ...`).
It also makes it harder to enable and disable all but a few nullability warnings: if you suppress one warning, then use `#nullable disable` and `#nullable enable` on a code block, you must re-iterate the suppression.
Let's now spell out the second design:
1. There is a nullable warning context as a separate "bit". It can be set by default at project-level, and overridden with `#nullable` directives. It needs to be on to get nullable warnings at a given point in the source.
2. Individual warnings continue to be able to be individually disabled at the project-level with `/NoWarn` and disabled or restored in source with `#pragma warning ...`.
3. Additionally there is a "virtual warning identifier", `nullable` that can be used in `/NoWarn` at the project level, and that individually disables all warnings related to the nullable feature.
This design allows common cases to be expressed simply:
- enable nullable warning context at the project-level with `<Nullable>` set to `enable` or `warnings`, then adjust in source with `#nullable` directive,
- enable nullable warning context in source with `#nullable` directive,
- `/NoWarn` continues to suppress warnings, including all nullable warnings using the `nullable` virtual identifier and the W-warning with `8600`, except when a specific warning is enabled in source with `#pragma warning enable <id>`.
**Conclusion**:
We're going to pursue the second design, based on "nullable warning context" bit.
We tentatively decided to remove support for the `nullable` group from the `#pragma warning ...` directive.
#### Names of options
We considered some alternative naming schemes, to try and clarify the options:
`warnings`/`annotations`
`warningsOnly`/`annotationsOnly`
`analysisOnly`/`annotationsOnly`
**Conclusion**:
There is a strong desire to use the same labels for the project-level (`<Nullable>...</Nullable>`) and source-level (`#nullable enable ...`) settings.
We decided to keep the current names for now: `warnings` and `annotations`. We plan to revisit that.
#### Removing support for `#pragma warning enable ...`
The `enable` verb in the `#pragma warning` directive seems a general feature, orthogonal to the nullable feature. We feel we need to review it and confirm how it interacts with various mechanisms for suppressing warnings.
**Conclusion**:
We will leave this feature for now and revisit it later, including the `nullable` group identifier.
#### Removing support for `safeonly` group identifier
Is the recent decision to remove `safeonly` and `safeonlyWarnings` still holding, despite some concerns raised?
**Conclusion**:
Yes, `safeonly` is okay to remove at this point. `/NoWarn:8600` has the same effect.