Closed MelbourneDeveloper closed 1 year ago
Youssef1313
commented
3 years ago The current compiler behavior is expected. See the note at var - C# reference:
When var is used with nullable reference types enabled, it always implies a nullable reference type even if the expression type isn't nullable.
See also LDM-2019-12-18.
Note that the compiler correctly tracks the state of the variable, i.e, it won't show a warning when you dereference it, unless you assign it a nullable value later on.
MelbourneDeveloper
commented
3 years ago @Youssef1313 Thanks but...
I guess a why is in order?
MelbourneDeveloper
commented
3 years ago Can I take it from the link above that the intention is to change the behavior in future?
If that's the case, won't a lot of existing code be changed?
Youssef1313
commented
3 years ago Can I take it from the link above that the intention is to change the behavior in future?
Nope, The conclusion of the LDM is that var? is not needed (var is effectively what var? was suggested to do).
Note that you won't get a warning if you don't assign it a nullable value.
See SharpLab for an example.
MelbourneDeveloper
commented
3 years ago @Youssef1313 but this is the problem, and the reason why I didn't end up just specifying string[] in the first place:
Perhaps this code rule needs adjusting for this scenario.
IDE0007
MelbourneDeveloper
commented
3 years ago Friends don't let friends violate code rules
MelbourneDeveloper
commented
3 years ago If I want to avoid this issue I have to disable warnings everywhere
MelbourneDeveloper
commented
3 years ago This is particularly nasty:
Youssef1313
commented
3 years ago For your scenario, I think you can use var safely. The compiler will know that Split doesn't return null, and won't warn. This satisfies both IDE0007 and the compiler.
MelbourneDeveloper
commented
3 years ago @Youssef1313 not gonna pretend that I completely understand why var creates a nullable type, but I can say with certainty that I expected it not to, and I expect that most other people will as well.
MelbourneDeveloper
commented
3 years ago For your scenario, I think you can use var safely. The compiler will know that Split doesn't return null, and won't warn. This satisfies both IDE0007 and the compiler.
@Youssef1313 Look above. It doesn't.
MelbourneDeveloper
commented
3 years ago Or, to put it another way...
I can't use var in this scenario because the quick info results are too confusing. So, I need to specify the type explicitly so it's clear what is going on.
If IDE0007 didn't kick in, in this case, it would be nice.
Youssef1313
commented
3 years ago I mean, yes var will change the type to string[]?. But it's not a problem for such a case (to me at least).
To be explicit about tokens being string[], I think IDE0007 needs to be adjusted for that.
Tagging @CyrusNajmabadi @sharwell for thoughts about IDE0007. I tend to agree that IDE0007 could be relaxed not to warn if the nullable annotation is different.
MelbourneDeveloper
commented
3 years ago @Youssef1313 personally, I'm not going to be able to sleep until the code rules force me to avoid var on this case, but that's just me.
I personally think there should be a new opt in code rule. I have to go back over all my code and without code rules I feel vulnerable.
CyrusNajmabadi
commented
3 years ago but I can say with certainty that I expected it not to, and I expect that most other people will as well.
Sure. but there's a good reason it allows null. So that code thta uses null with those variables today continues to work without problems. We don't want people with correct code, that does use null, and which does use var to have issues migrating to NRT.
Note that the compiler performs flow analysis here. So if you do end up assigning a nullable value, and then you do something null-unsafe (like index/access through that value) we will warn.
CyrusNajmabadi
commented
3 years ago I tend to agree that IDE0007 could be relaxed not to warn if the nullable annotation is different.
I'm not seeing the issue here really. What is the actual problem that that would solve? I get that tehre is an initial reaction that i'm hearing that the user feels it's wrong that the type is inferred as the nullable form. However, that was intentional on our part as we found that it supports the most amount of user code, with the same net results in terms of null safety.
Indeed, we originally started with the more restrictive form, where the type was inferred to be the non-null variant. This turned out to be non-viable as so much null-safe code now would get NRT warnings saying that it was not ok to assign null into these variables.
We intentionally switched this because there was no benefit to that restrictive form, and there were substantive negatives.
CyrusNajmabadi
commented
3 years ago I guess a why is in order?
To clarify:
This is done because it accurately represents the meaning of code as it existed before and supports the patterns and practices that people have used correctly surrounding variables and hte use of null with them. The goal of NRT is not to prevent nulls. The goal of NRT is to prevent null reference exceptions. Nulls themselves are fine, and are indeed common in .net/c#. What is not fine is when a null gets into an undesired and unexpected location and causes a null-ref at runtime.
With fields, and inter-procedural members, we have no way to understand or model the flow. So we have to take the signatures as is and model things concservatively in this fashion. that's whyit's necessary to really mark what can and can't be null across boundaries of methods.
However, with locals, and much of the intra-procedural code that a user has, we can understand and model the flow state with reasonable accuracy. As such, we can allow a broader interpretation that allows for null, but which then reports issues with that with the code if it exists. Specifically, we can identify where you would get the null reference exceptions, which was the primary goal of NRT in the first place.
CyrusNajmabadi
commented
3 years ago I personally think there should be a new opt in code rule.
The var rule is working properly here. The rule has always been if var can be used because it will not change code semantics. In this case, semantics of the code do not change. That's a critical part of the design of NRT. It has been designed since day 1 that it has no semantic effect on the code. this was necessary so that all code could start using it and be 100% certain there would be absolutely zero impact on existing programs and how they work.
I have to go back over all my code and without code rules I feel vulnerable.
The core issue here is that you have interpreted this to make you feel that you are vulnerable. This is not the case. The compiler is performing the same analysis in either event. The only distinction here is that it performs more precise flow analysis in the case of var that calls out the point where you would actually encounter a problem, as opposed to a prior point where no problem would necessarily happen. The latter is unfortunate as it means completely normal and useful coding patterns become warnings (despite having nothing wrong with them) as opposed to the former approach (which only actually warns in the case where something bad might actually happen).
There is no need to feel vulnerable here :)
MelbourneDeveloper
commented
3 years ago I'm not seeing the issue here really.
@CyrusNajmabadi
Very simply, this is misleading:
And it has a cascading effect...
Everything that uses this variable also looks as though it is nullable when technically it's not.
MelbourneDeveloper
commented
3 years ago I'd be telling everyone in the team to switch to the explicit type in this case so the quick info is correct and clear.
MelbourneDeveloper
commented
3 years ago Personally, I'd like a code rule to enforce this ^^^
CyrusNajmabadi
commented
3 years ago Very simply, this is misleading:
I'ts not misleading. It is exactly the rules of the language :)
And it has a cascading effect...
It should not. Can you give an example of what sort of cascading effect this causes?
Everything that uses this variable also looks as though it is nullable when technically it's not.
That's not how NRT works. We use flow analysis to actually make the decisions at the point where that varaible is used to make an accurate assessment of the null safety of that particular reference point.
CyrusNajmabadi
commented
3 years ago I'd be telling everyone in the team to switch to the explicit type in this case so the quick info is correct and clear.
I would not recommend this. The quick info here is literally 100% accurate. It is exactly how the compiler sees the variable as per the rules of the language. The type of that variable is string[]? according to the language. The disconnect here is that NRT is not simply a system for designating types of things. NRT is a system to try to prevent null-ref-exceptions. In particulr, null-ref-exceptions come from null values, not nullable types. In that regard, the value of NRT comes from a flow analysis system that tracks if a null value is being used inappropriately. That will happen orthogonally to what the types are designated as.
MelbourneDeveloper
commented
3 years ago It should not. Can you give an example of what sort of cascading effect this causes?
Actually, you are correct. It doesn't. I thought it did, but it was just the var issue again.
CyrusNajmabadi
commented
3 years ago Personally, I'd like a code rule to enforce this
This would be an anti pattern. The style rule and IDE tooling is both accurately conveying the meaning of hte language, and accurately following the rule here, while also ensuring code semantics are maintained :)
CyrusNajmabadi
commented
3 years ago Actually, you are correct. It doesn't. I thought it did, but it was just the var issue again.
Understood :) i think a core thing here is that you have equated nullable types with nullable values. They are separate concerns. The type system here has no problem with nullable types, as the flow system is the one that is actually ensuring that nullable values aren't flowing into places they shouldn't.
As the language makes no distinction between a nullable or non-nullable type (which is necessayr to ensrue that you can start using NRT with no semantic changes), there should be no cascading changes or semantic issues that could arise from this. If that were possible, then people could not upgrade to NRT without it potentially changing their code meaning :)
Youssef1313
commented
3 years ago @CyrusNajmabadi Based on this discussion, I think you'll disagree with https://github.com/dotnet/docs/issues/22997. You may want to put your thoughts their so that the docs don't get updated badly.
MelbourneDeveloper
commented
3 years ago @Youssef1313 @CyrusNajmabadi
In a nutshell, the reason why I will always use the explicit type instead of var in the future is this compiles:
Up until now, I assumed it wouldn't, and I believe that this opens up a lot of possible issues. So, I will change code like this, in order to stop anyone from setting the variable to null:
I would like to see an opt-in code rule that forces this.
CyrusNajmabadi
commented
3 years ago In a nutshell, the reason why I will always use the explicit type instead of var in the future is this compiles:
Yes. that code is 100% safe. Why would we want it to not compile. As stated above, the goal of NRT is not to make legal code not compile. The goal of NRT is to prevent null reference exceptions. There is no null ref exception in your code... so why would we want it to be disallowed?
and I believe that this opens up a lot of possible issues.
What issues does this open up?
So, I will change code like this, in order to stop anyone from setting the variable to null:
I think you are confusing null values with null-reference-exceptions. Null values are not a problem in and of themselves. Indeed, they are used hugely in teh .net ecosystem and are not an issue as long as you don't use them incorrectly. NRT is about preventing you from misusing them. It's not about preventing nulls from existing at all in code.
I think you have a misunderstanding of waht the NRT feature is and what it is intended to be used for :)
CyrusNajmabadi
commented
3 years ago so that the docs don't get updated badly.
Yes. I've let them know to include me in any discussions on that. That potential advice would not be advised by us. Thanks!
CyrusNajmabadi
commented
3 years ago I would like to see an opt-in code rule that forces this.
What value would this code rule provide? I've seen several times a concern stated that there is an issue here and that this code should not be allowed. however, i haven't seen a justification provided. In the cases you've mentioned, there is no null-ref happening. And the code is totally clear, reasonable, and safe. So why would these rules not apply? Or, put another way, why would we add a rule to disallow something that isn't a problem? :)
MelbourneDeveloper
commented
3 years ago @CyrusNajmabadi
I understand what you are saying. The code allows null. There is no confusion here.
What I'm saying is that I will be changing to the explicit type so as to avoid a nullable variable.
In some cases I might not want that and I would declare it as nullable, but even then, I think it should be explicit - that's the point of NRT : to express intention.
MelbourneDeveloper
commented
3 years ago @CyrusNajmabadi
NRT is about expressing intent.
If some construct is going to make a variable nullable, I need to know about it. I should be forced to decide.
CyrusNajmabadi
commented
3 years ago What I'm saying is that I will be changing to the explicit type so as to avoid a nullable variable.
My point is specifically that that provides no value (no pun intended :D). Null values are not the problem. And preventing null values has never been the goal of NRT. Preventing null-crashes has been the goal.
that's the point of NRT : to express intention.
That's not the case, no. The goal of NRT is to prevent null-crashes. Not only should null safe code not be flagged, but it's an anti-goal to "be explicit". Because now you're punishing safe code by forcing it to be explicit when the language already knows it's safe.
I should be forced to decide.
We explicitly disagree. That was considered for NRT and decided explicitly that it is not actually how we would want this to work. We don't wnt users to have to jump through hoops to state something the language can already decide. It's just extra work for less functionality.
It feels lke you have an issue with the language design here. but in that case, requests to change that should go to dotnet/csharplang.
MelbourneDeveloper
commented
3 years ago No. That's the point of an opt in code rule.
Many won't want the rule and that's their choice. I do because this is going to confuse the hell out of me.
CyrusNajmabadi
commented
3 years ago NRT is about expressing intent.
Just to be totally clear on this point. This is not the case. NRT is not about expressing intent. NRT is about trying to prevent null-crashes. Null crashes come from null-values flowing into locations that cannot handle them. Null-values flowing can be determined without the need to express any intent.
We explicitly considered what you have described and viewed it as an anti pattern. That's because at the level of intra-proedural analysis, intent isn't needed as flow analysis far more accurately describes what is actually happening (which serves the goal of preventing null value crashes).
At hte inter-procedural level though, what you are saying comes into play. As there is no facility in the language for flow analysis at all at that level, we have to fall back to a conservative view based on type-analysis. Here, we can't figure out much of anything, so we depend on the user to express intents at the type-system level. in effect, it's a 'poor mans' solution to the problem, but the best we can come up with given the limits of the language and the .net platform.
spydacarnage
commented
3 years ago I think you're misunderstanding a point made by Mads when NRT first came out. He often said in interviews that NRT was there to help the developer express intent. However, that was at member level, not for local variables.
So, adding a nullable annotation to the MiddleName property of my Person class (the classic example) shows me, as a user of your class, your intent to say that the MiddleName property is not needed.
But in your local method, when you have that instance, what you do with a local instance of the MiddleName property is up your use, not the intent of the author.
CyrusNajmabadi
commented
3 years ago I do because this is going to confuse the hell out of me.
I don't think a rule will help here. It would seem to imply something different about the language and NRT than what is actually going on. That's why i think it would be a bad idea, and why we considered things in this space an "anti pattern". Because it works to subvert an actual understanding of what is happening. Fundamentally, there seems to eb a conflation of types and values here, and a belief " that this opens up a lot of possible issues,". However, that's not the case here :)
MelbourneDeveloper
commented
3 years ago Well sorry for putting my view forward.
Obviously, I'm a moron if this an anti-pattern
CyrusNajmabadi
commented
3 years ago Obviously, I'm a moron if this an anti-pattern
You are not a moron, and no one here has attempted to imply that at all. There is simply a misunderstanding here around the goals of NRT, and how types and values play into the design here. Please see my post above on intra vs inter-procedural analysis :)
Well sorry for putting my view forward.
There's nothing to be sorry about. As i mentioned though, this is better taken as a dsicussion on dotnet/csharplang as this really is a fundamental discussion about what NRT is deeply at a language level.
Note: we have other arenas for this sort of discussion as well. For example, gitter.im/dotnet/csharplang (or dotnet/roslyn), and discord.gg/csharp. I'm happy to continue discussing this here or there with you, however you prefer :)
In no way is anyone saying you're a moron. There is just an attempt to clarify what seems to be an mismatch in the views here around what the language intends to provide versus what is wanted from the lang. In this case, this topic was investigated in depth, and we actually moved entirely away from a type-system-based-view of things here precisely because it didn't match up with reality, and it made it costly and painful to move correct code over to NRT. That went again another large goal we had here which was that we wanted to avoid unnecessary friction unless totally necessary.
Having var infer the nullable type of the initializer absolutely added major friction for no good purpose. It did not prevent any null-ref cases. And it meant lots of completely safe and totally idiomatic code now warned. Again, this was not speculative. We encountered it immediately across every codebase we attempted to experiment with NRT with. The approach we settled on:
this approach ended up being effectively superior on almost all axes, and so it was taken.
canton7
commented
3 years ago Skimming the thread, I haven't seen anyone lay out the following explicitly, so I'm just going to leave this here. Apologies if this has already been covered in clear terms -- please ignore if so.
The compiler can accurately track the nullable "flow state" of every local variable (i.e. whether it can currently be null or not), because it has lots of information.
public string M()
{
string? foo = null;
// Warning -- the compiler knows that 'foo' can be null, and so accessing 'foo.Length' warns
Console.WriteLine(foo.Length);
foo = "Hello, World";
// No warning -- even though 'foo' is declared as nullable, the compiler knows that it can't be null here
Console.WriteLine(foo.Length);
// No warning -- even though 'foo' is 'string?' and this method returns 'string', the compiler knows that 'foo'
// can't be null here, and so this is safe
return foo;
}See SharpLab.
This is why it is safe for the compiler to infer 'var foo' as a nullable type -- it accurately knows when the contents of that variable 'foo' can and can't be null. It will let you call methods on 'foo' when it knows that 'foo' can't be null, and it warns you at points where 'foo' can be null. The warnings happen at the point that a NullReferenceException would be thrown, or where the value escapes from the method (returned, passed to another method, etc).
IIRC there was even a discussion on whether to allow local variables to be declared as nullable/non-nullable at all: it's pretty much pointless since the compiler has all of the information it needs to accurately tell you when a variable can and can't be null. The place where you need to start explicitly annotating things is when specifying contracts, where you're making promises to other users of your types.
sharwell
commented
3 years ago I was one of the people who advocated for "expressing intent" to be considered in this design. My argument was based on a claim that a developer writing var is intending to say "use flow analysis to choose an appropriate static type for this variable that works throughout the method". Since nullability is a flow analysis concept, it's always safe to choose the nullable form where var is used and it works in strictly more cases than the non-nullable form. A developer interested in an exact static type is more likely to prefer an explicit type instead of var, so a design decision that optimizes var for the flow analysis scenario is unlikely to negatively impact users who prefer explicit types.
As with any decision of this scale, there are always going to be a few users that don't fit cleanly on one side or the other. However, I do believe that the current design does a great job of optimizing var for the "choose the best static type from flow analysis" intent and optimizing explicit local types for the "use this specific static type" intent.
Version Used:
See below
Steps to Reproduce:
UrlExtensions -> ToRelativeUrl
Expected Behavior:
tokensshould bestring[]Actual Behavior:
tokensisstring[]?