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.

Sign up for GitHub

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

Jump to bottom

Track nullability in script code #33096

Merged
merged 13 commits into from
Mar 26, 2019
Merged

Track nullability in script code #33096

merged 13 commits into from
Mar 26, 2019

Conversation

filipw
Copy link
Contributor

@filipw filipw commented Feb 4, 2019 •
edited
Loading

At the moment script code is explicitly excluded from nullability analysis.

So the following script doesn't produce any warnings:

#nullable enable
string foo = null

This PR includes script code into the nullability analysis.
Additionally, since scripting doesn't expose an API to set Language Version, I bumped it to C# 8 (since it should always be the newest). Once C# 8 ships, we could move it back to Latest.

@filipw filipw requested review from a team as code owners February 4, 2019 18:44
@filipw filipw force-pushed the feature/script-nullable branch from b5f894d to 16d8555 Compare February 5, 2019 07:46
@filipw filipw changed the title Perform nullable reference analysis in loose script code Track nullability in loose script code Feb 5, 2019
@jinujoseph jinujoseph added Area-Interactive Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Feb 5, 2019
@filipw
Copy link
Contributor Author

filipw commented Feb 5, 2019

rebased this on top of master

jaredpar
jaredpar requested changes Feb 5, 2019
View reviewed changes
Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't move C# scripting to be C# 8.

@filipw filipw force-pushed the feature/script-nullable branch from 228d805 to 67391af Compare February 7, 2019 12:13
@filipw
Copy link
Contributor Author

filipw commented Feb 7, 2019

@tmat I have updated the PR so that scripting uses LanguageVersion.Latest again, like @jaredpar requested.
I also came up with a solution of setting LanguageVersion via ScriptOptions which I think makes sense and - as we already discussed here - could be quite useful. It also enables nullability analysis for scripting.
If you have some time, let me know if we could go forward with such approach - thanks!

@filipw filipw force-pushed the feature/script-nullable branch from 25a7e91 to 11321d2 Compare February 11, 2019 08:24
@filipw filipw changed the title Track nullability in loose script code Track nullability in script code Feb 11, 2019
@filipw filipw force-pushed the feature/script-nullable branch from 11321d2 to 98f044b Compare February 11, 2019 19:04
@tmat tmat mentioned this pull request Feb 15, 2019
Open
@filipw filipw force-pushed the feature/script-nullable branch from 98f044b to 27ea453 Compare March 4, 2019 15:56
@filipw
Copy link
Contributor Author

filipw commented Mar 4, 2019

I rebased this on top of master and resolved conflicts that appearee due one of the recent merges.

@filipw filipw force-pushed the feature/script-nullable branch from 19fb5b7 to a41653e Compare March 4, 2019 21:19
tmat
tmat requested changes Mar 4, 2019
View reviewed changes
Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕐

@filipw
Copy link
Contributor Author

filipw commented Mar 4, 2019

I also noticed - unrelated to this PR - a formatting warning IDE0055 on this line, which was introduced by #32668, so I fixed that too.

tmat
tmat approved these changes Mar 4, 2019
View reviewed changes
Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@filipw
Copy link
Contributor Author

filipw commented Mar 6, 2019

@jaredpar is this OK for you?

jaredpar
jaredpar approved these changes Mar 7, 2019
View reviewed changes
Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@filipw filipw mentioned this pull request Mar 10, 2019
Closed
@filipw
Copy link
Contributor Author

filipw commented Mar 26, 2019

I resolved the conflicts after #29069 was merged. The integration test seems flaky (failure unrelated to the PR).
Given that everyone signed off, is it OK to get this merged?

@cston
Copy link
Member

cston commented Mar 26, 2019

Thanks @filipw. Sorry for the delay.

Would you mind if I squash commits when merging?

@filipw
Copy link
Contributor Author

filipw commented Mar 26, 2019

Would you mind if I squash commits when merging?

sure no problem, it's gotten a bit messy indeed 😅

@cston cston merged commit 7f809ef into dotnet:master Mar 26, 2019
@cston
Copy link
Member

cston commented Mar 26, 2019

Thanks @filipw for the PR!

@filipw filipw deleted the feature/script-nullable branch March 26, 2019 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cston cston cston approved these changes

@jaredpar jaredpar jaredpar approved these changes

@tmat tmat tmat approved these changes

Assignees

@tmat tmat

Labels
Area-Compilers Area-Interactive Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@filipw @cston @tmat @jaredpar @jinujoseph @jcouv