Do not let 'null' children slip into the AST #5446
Conversation
| protected CollectionType(Type arg) { | ||
| this.arg = arg; | ||
| this.TypeArgs = new List<Type> { arg }; | ||
| TypeArgs = new List<Type>(1); | ||
| if (arg != null) { | ||
| TypeArgs.Add(arg); | ||
| } |
There was a problem hiding this comment.
Why would CollectionType ever be called with null? (Same for the next constructor.)
I imagine that many places in the code expects a CollectionType to have 1 (or 2, for a map) type arguments. So, allowing a null argument to the constructor would just seem to delay the crash.
There was a problem hiding this comment.
The parser passes in null when the type argument is left out, for different collection types, for code such as:
function Foo(): seq
I imagine that many places in the code expects a CollectionType to have 1 (or 2, for a map) type arguments. So, allowing a null argument to the constructor would just seem to delay the crash.
If arg == null, then the resolver will attempt to infer type arguments and insert them into TypeArgs.
I doubt this is a reliable design but I didn't want to change it. I think better might have been to have a class OmittedType : Type that is used instead of null, in cases where the user omits the type arguments.
Source/DafnyCore/AST/Types/Types.cs
Outdated
| @@ -1949,16 +1950,20 @@ public abstract class CollectionType : NonProxyType { | |||
| Contract.Requires(1 <= this.TypeArgs.Count); // this is actually an invariant of all collection types | |||
Source/DafnyCore/AST/Types/Types.cs
Outdated
| this.TypeArgs[0] = arg; | ||
|
|
||
| Debug.Assert(TypeArgs.Count == 0); | ||
| TypeArgs.Add(arg); | ||
| } | ||
| public virtual void SetTypeArgs(Type arg, Type other) { | ||
| Contract.Requires(arg != null); | ||
| Contract.Requires(other != null); | ||
| Contract.Requires(this.TypeArgs.Count == 2); |
I'm not sure what you're asking. This PR does not relate to concurrency issues, so the tests are not testing for them. The test |
|
Sure, but is it the case that only the tests that explicitly test for concurrency issues cause the issues we frequently experience? |
No it's not. Instability in tests is not limited to any particular test, and it can and does also occur outside of IDE tests. However, it most frequently occurs in IDE tests because the IDE has many concurrent parts. Instability in IDE tests usually comes from a bug in the IDE code, not the test, but sometimes also comes from the test. There are particular IDE tests that are more liable to instability, namely those that tests performance in some way. These tests generally only pass if a particular performance threshold is met, and will retry up to X times until they meet that threshold. We protect ourselves against making changes, either in tests or in any other location, that cause IDE test instability, by running the IDE tests twice in the build. I think it would be good if we run them more often but obviously there's a trade-off there. |
|
That's probably a tangential discussion, but it seems to me that we have a decent idea about which PRs are more likely to introduce instability. Maybe we should try being more targeted about where we do repeat runs. If we do that, we could even afford doing more runs where necessary. |
Fixes #5357, #4129
Description
nullfrom being returned byNode.ChildrenHow has this been tested?
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.