var should infer nullable type #40755
Conversation
jcouv
added
Area-Compilers
Feature - Nullable Reference Types
jcouv
force-pushed
the
nullable-var
branch
4 times, most recently
from
f58c3b0 to
7e0227e
Compare
jcouv
added
the
PR For Personal Review Only
jcouv
removed
Blocked
PR For Personal Review Only
There was a problem hiding this comment.
😟 Did this just go from O(1) to O(n)? #Resolved
There was a problem hiding this comment.
There was a problem hiding this comment.
❔ Was this just a test or was it needed for some reason? #Resolved
There was a problem hiding this comment.
Explained in OP: Filed issue #40821 to track the remaining issue with analysis of local functions which affected bootstrapping for this PR.
The code above behaves like Solution? solution = ...; which hits a problem with analysis of local functions. Most such problems were fixed by Andy yesterday, but one or two remained, which I worked around to unblock this PR (because it's not a new problem).
In reply to: 364502291 [](ancestors = 364502291)
|
Could you please prepare a corresponding PR to adjust a specification in |
There was a problem hiding this comment.
ToAnnotatedTypeWithAnnotations [](start = 76, length = 30)
We should consider not changing this one. The foreach variable is readonly, so nobody's going to be assigning null to it. #Resolved
There was a problem hiding this comment.
Discussed in more details in another thread in this PR. Tagged you there. #Resolved
There was a problem hiding this comment.
This should only be done for variables declared with a var pattern. The others should stay non-null. #Resolved
|
Finished reviewing (Iteration 4). I did not review tests deeply because I fear fixing the treatment of pattern variables may change the tests a lot. #Resolved |
jasonmalinowski
left a comment
jasonmalinowski
left a comment
There was a problem hiding this comment.
Still trying to sort out the IDE changes. (Making progress though!)
|
@jasonmalinowski the PR no longer has logic change in the IDE (only tests and two explicit types for bootstrap issue) #Resolved |
jasonmalinowski
left a comment
jasonmalinowski
left a comment
There was a problem hiding this comment.
Thanks for the comprehensive IDE testing on UseExplicitType. There was one test that looked like it was asserting the wrong thing, and one also that was asserting the wrong thing which you did know about but there's a difference of how the compiler and IDE teams handle tests like that. Comments in that file.
We also should have some tests probably too on UseImplicitType to make sure we're not converting things to var if they're non-nullable, but I'm happy to file a tracking item to do that separately since I suspect any bugs you find there are existing bugs and not new bugs with this change.
There was a problem hiding this comment.
This looks like you're now testing #37309, so you may want to close that too. #WontFix
There was a problem hiding this comment.
There's probably other code/tests tagged with this issue. I'll leave that separate for now.
In reply to: 367219324 [](ancestors = 367219324)
There was a problem hiding this comment.
Also test typeInfo.Type.NullableAnnotation and typeInfo.ConvertedType.NullableAnnotation, or is that automatic at this point? #Resolved
There was a problem hiding this comment.
Does the throw null! matter here for some reason? #Resolved
There was a problem hiding this comment.
I don't think the throw null! matters, but code that doesn't compile makes me uncomfortable ;-) #Resolved
There was a problem hiding this comment.
Shouldn't this be (Program? y1)? Or otherwise this is assigning a nullable x to a non-nullable y1? #Resolved
There was a problem hiding this comment.
(I'm not going to block this PR on this concern since this is an edge case, but I am curious!) #Resolved
There was a problem hiding this comment.
This is capturing existing behavior. I'll update to capture desired behavior instead, as you indicated.
In reply to: 367220385 [](ancestors = 367220385)
There was a problem hiding this comment.
The IDE has a different pattern than the compiler here: make the test assert what should happen and then skip the test by putting the bug number in the skip attribute. We avoid having tests assert the wrong thing because it often means somebody who accidentally fixes the bug might think they made things worse, and not realize they actually made things better. The counter-argument of "but you want to know about further changes" we don't usually worry about. #Resolved
|
Moved this PR to 16.6p1 branch because of concerns with possible implications to other test scenarios. #Resolved |
|
Rebased on top of dev16.6-preview1 and will merge when green. |
7 participants
Fixes #40477 based on recent LDM decision.
Fixes #31874
Blocked on #40422 (which fixes a bug with analysis of local functions which prevents bootstrapping)Filed issue #40821 to track the remaining issue with analysis of local functions which affected bootstrapping for this PR.