-
Notifications
You must be signed in to change notification settings - Fork 257
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.
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
Do not let 'null' children slip into the AST #5446
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we make sure those new tests introduce new concurrency issues?
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outdated?
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outdated?
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
null
from being returned byNode.Children
How has this been tested?
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.