Enable nullable annotations in language types in Scripting#351
Enable nullable annotations in language types in Scripting#351slozier merged 1 commit intoIronLanguages:mainfrom
Conversation
| public Stream InputStream { get { InitializeInput(); return _inputStream; } } | ||
| public Stream OutputStream { get { InitializeOutput(); return _outputStream; } } | ||
| public Stream ErrorStream { get { InitializeErrorOutput(); return _errorStream; } } | ||
| public Stream InputStream { get { InitializeInput(); return _inputStream!; } } |
There was a problem hiding this comment.
Maybe this change is slated for later, but you could use [MemberNotNull(nameof(_inputStream), nameof(_inputReader), nameof(_inputEncoding))] on InitializeInput ans friends.
There was a problem hiding this comment.
I thought about it, but to mark those member fields MemberNotNull, InitializeInput (and friends) would have to change to really guarantee to the compiler they are never null on return. This means that the conditions on line 103 and 105 would have to change to if (_inputStream is null || _inputEncoding is null || _inputReader is null). Since the first test (L103) is executed on every property access, it means that every time the test would take 3 times as much as before — just to make the compiler happy about nullability guarantees that are obvious for the human reading the code (and maybe to some AI agents 😄 ). In the end I thought it was not worth the change, and the null-forgiving operator is justified. What is nice is that the whole anaysis can be done locally; it does not require looking up any other source file.
Generally, I want to do changes file-by-file, in file groups, to make the review easier and for my own well-being, so there is nothing to be slated for later. If I submit a file for a review, that is it, if something looks questionable, suspicious, or unclear, fire right away.
| /// it returns false. | ||
| /// </summary> | ||
| bool TryGetValue(out dynamic value); | ||
| bool TryGetValue(out object? value); |
There was a problem hiding this comment.
I don't really know what the consequences of changing from dynamic to object are. If it wasn't an accidental change I assume you've thought it through and have a good reason for it. 😄
There was a problem hiding this comment.
I am not sure about this one. The reason I changed it to object? is because SetValue below accepts object?. It seems that the compiler is totally happy with assignment of object to dynamic without casting, as well as dynamic to object without casting. So it treats them equivalently?
One can argue that dynamic has more capabilities that object so perhaps it should be treated as a specialized object (as if a subtype). We can decide on this as a rule — in such case dynamic? is more appropiate. If so, there are possibly more places where I need to change things (including future planned PRs).
In TryGetValue the type is out, and in SetValue it is in, so it is as if co-variant vs. variant typing, but because the type is actually not changed, it felt weird to set an object into the scope, just to retrieve it later magically as dynamic.
I need to sleep on this.
There was a problem hiding this comment.
The type conversion is neither clear not intuitive. Intuitively, the the type of object put into the scope should be the same as the type of the object retrieved from it. This means object/object or dynamic/dynamic. But there is no reason for the scope to hold onto the dynamic type; it doesn't perform any dynamic operations on it. Therefore type object seems more appropriate.
From the caller's perspective it changes nothing: you can still declare the retrieved value as dynamic if you want to. For instance, this works:
if (scopeVariable.TryGetValue(out dynamic? variable) {
...
}and obtain a variable typed as dynamic.
So in the end, I would not make any changes here.
2 participants
Activates creation of additional polyfills
MaybeNullAttributeandNotNullWhenAttribute.