Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Nullable build setting #4058

Closed
jcouv opened this issue Sep 27, 2018 · 16 comments
Closed

Add support for Nullable build setting #4058

jcouv opened this issue Sep 27, 2018 · 16 comments

Comments

@jcouv
Copy link
Member

@jcouv jcouv commented Sep 27, 2018

This is a tracking work item related to the "nullable reference types" feature in C# 8.0 (for dev16).
We'll add design notes here as it crystallizes (is it a property or some other form of API?)
Tagging @davkean

@jcouv
Copy link
Member Author

@jcouv jcouv commented Nov 3, 2018

The design for turning on the nullable feature changed since our last discussion.
Previously, we would have [NonNullTypes] (for class- or method-level control) or [module:NonNullTypes] (for module/project-level).
Now, we'll have #nullable ... for scoped control in source, and a compiler flag for project-level control. That compiler flag is exposed via MSBuild property NullableReferenceTypes, as part of dev16 preview1.

Based on this, there are two scenarios of interest:

  1. exposing a checkbox in Build options ("Enable Nullable Reference Types")
  2. allowing a codefixer to change this project property (like UpgradeProject does for LangVersion)

@davkean I assume that those scenarios translate into three work items:

  1. some common work in project-system (object model or API change)
  2. IDE work to add Build option UI (or is this also owned by project-system)
  3. codefixer work (IDE/myself)

@davkean @Pilchie Do you think the API work (1) could be reasonably scheduled as part of preview 2? If yes, I would do the codefixer (3) in preview 2 as well.

Loading

@jcouv jcouv changed the title Add support for module-level NonNullTypes Add support for NullableReferenceTypes build setting Nov 3, 2018
@jcouv
Copy link
Member Author

@jcouv jcouv commented Nov 12, 2018

Tagging @tmeschter per email discussion about preview2 goals. Thanks

Loading

davkean added a commit to davkean/project-system that referenced this issue Nov 15, 2018
Fixes configuration property portion dotnet#4058

This can be acccesed using:

Project.ConfigurationManager.ConfigurationRow(configurationName).Item(1).Properties["NullableReferenceTypes"]
davkean added a commit to davkean/project-system that referenced this issue Nov 15, 2018
Fixes adding configuration-specific property for dotnet#4058

Note: This is a configuraiton-specific property but writes itself without a configuration condition, similar to LangVersion. We need this because it's going to live on the Build property page which pulls from configuration.

You can access this using DTE.Configuration.Properties["NullableReferenceTypes"]. We do not yet have a strongly typed API for it and will do that when legacy adds support for it.
@etbyrd
Copy link
Contributor

@etbyrd etbyrd commented Nov 19, 2018

I could get a PR out for the checkbox in the Build property page - do we have an idea of where on the page we would want it?

Loading

@jcouv
Copy link
Member Author

@jcouv jcouv commented Nov 19, 2018

For adding the checkbox to the Build property page, I think putting it under "Check for arithmetic overflow/underflow" would make sense.
@davkean Does that sounds ok?

image

Loading

@tmeschter
Copy link
Contributor

@tmeschter tmeschter commented Nov 19, 2018

I wouldn't want to add the checkbox without adding the underlying property support to csproj/msvbprj, and that's the expensive part.

Loading

@chucker
Copy link

@chucker chucker commented Dec 5, 2018

In my testing, it looks like the <NullableReferenceTypes> property only works with new-style projects. Are there plans to make it work with the old project system (e.g., for an ASP.NET project which can't be migrated due to #2670)?

Loading

@jcouv
Copy link
Member Author

@jcouv jcouv commented Dec 6, 2018

@chucker Yes, this property will be made to work in old project system too, before C# 8 ships as RTM, but likely after dev16.0 (where C# 8.0 language features are in beta).
The present issue tracks this remaining work.

Loading

@jcouv
Copy link
Member Author

@jcouv jcouv commented Dec 19, 2018

@davkean We had to modify the options to add a third one (true/false were not enough) and changed the name of the option in the process.
I assume that the change of name affects project system, but the change of values doesn't. Let me know if that's not the case, as one of the options might still change.
Sorry for the randomization.

Through msbuild the context could be set by supplying an argument for a "NullableContextOptions" parameter of Csc build task. Accepted values are "enable", "disable", "safeonly", or null (for the default nullable context according to the compiler). The Microsoft.CSharp.Core.targets passes value of msbuild property named "NullableContextOptions" for that parameter.

https://github.com/dotnet/roslyn/blob/master/docs/features/nullable-reference-types.md#setting-project-level-nullable-context

Loading

@Tragetaschen
Copy link

@Tragetaschen Tragetaschen commented Feb 5, 2019

Given how I spent the last thirty minutes, it might be worth going through the existing blogs and docs for C# 8 and modify the samples on how to set up that feature. That property name change and the new option names took quite some time to figure out.

Some examples are https://docs.microsoft.com/en-us/dotnet/csharp/tutorials/nullable-reference-types or https://blogs.msdn.microsoft.com/dotnet/2018/12/05/take-c-8-0-for-a-spin/

Loading

@jjmew jjmew removed this from the 16.0 milestone Feb 13, 2019
@jjmew jjmew added this to the 16.X milestone Feb 13, 2019
@jjmew jjmew removed this from the 16.0 milestone Feb 13, 2019
@jjmew jjmew added this to the 16.X milestone Mar 18, 2019
@jjmew
Copy link
Contributor

@jjmew jjmew commented Mar 19, 2019

Loading

@jcouv jcouv changed the title Add support for NullableReferenceTypes build setting Add support for Nullable build setting May 2, 2019
@jcouv
Copy link
Member Author

@jcouv jcouv commented May 2, 2019

From LDM discussion today (5/1/2019) including Phillip, we concluded that "Nullable" would be a better name than either "NullableReferenceTypes" or "NullableContextOptions". It should remain a string (to accept "enable", "disable" and a couple of other values).

Relates to compiler work item dotnet/roslyn#35432

Loading

@jnm2
Copy link

@jnm2 jnm2 commented Sep 30, 2019

Moved bug report to separate issue: #5551

Loading

@jcouv
Copy link
Member Author

@jcouv jcouv commented Jan 18, 2020

@drewnoakes @chucker @tmeschter @davkean I noticed this old issue. I assume that we still want to expose this checkbox. What release should we aim for?

Loading

@drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Jan 18, 2020

@jcouv I took a look at this but hit a block that would require help from @davkean or someone else who knows the details of the property pages better than me.

In short, adding the UI was straightforward, however we only want to enable that UI when LangVersion is 8 or greater. Sniffing the current version using the APIs available in the property pages (that must work with both legacy and CPS projects) gave me some trouble.

Loading

@chucker
Copy link

@chucker chucker commented Jan 19, 2020

@drewnoakes regarding legacy projects, I take from #5551 (comment) that this is no longer planned, so the UI can focus on CPS.

(I'd love to be wrong, though.)

Loading

@drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Jan 20, 2020

PR #5807 is up to fix this.

Loading

@drewnoakes drewnoakes removed this from the 16.x milestone Jan 20, 2020
@drewnoakes drewnoakes added this to the 16.5 milestone Jan 20, 2020
@drewnoakes drewnoakes self-assigned this Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

8 participants