-
Notifications
You must be signed in to change notification settings - Fork 274
Fix: Recursive constant initialization was not checked if in constructor #2862
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
base: master
Are you sure you want to change the base?
Conversation
This PR fixes #2727 by adding an appropriate error message if constants are initialized in the wrong order.
Source/DafnyCore/DafnyCore.csproj
Outdated
@@ -3,7 +3,7 @@ | |||
<Target Name="RunCoco" BeforeTargets="PreBuildEvent" Outputs="$(ProjectDir)Parser.cs;$(ProjectDir)Scanner.cs" Inputs="$(ProjectDir)Dafny.atg"> | |||
<Exec Command="dotnet tool restore" /> | |||
<Exec Command="dotnet --info" /> | |||
<Exec Command="dotnet tool run coco $(ProjectDir)Dafny.atg -namespace Microsoft.Dafny -frames "$(ProjectDir)Coco"" /> | |||
<Exec Command="dotnet tool run coco "$(ProjectDir)Dafny.atg" -namespace Microsoft.Dafny -frames "$(ProjectDir)Coco"" /> |
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.
This has nothing to do with this PR, but I tried having dafny in a folder with space in its name and I had to fix this to make it work.
@@ -2846,7 +2846,7 @@ public class ModuleBindings { | |||
} | |||
|
|||
// ---------------------------------- Pass 1 ---------------------------------- | |||
// This pass: | |||
// This pass or phase: |
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.
I keep forgetting that these are called "pass", I'm always looking for "phase". Next time, if I forget again, I will be able to find them.
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.
Nice! The changes look good, but I wonder if we could reuse more of the other code. In particular:
- We already have code to detect loops in constant initialization, as in
class C { const a := b; const b := a }
, so maybe we could reuse it - Even if we can't reuse that code, we already have code to find cycles and traverse ASTs, so I don't think we should re-implement the graph traversal; instead, we can use a visitor to collect all field references in the RHS of a variable, and the SCC checker to find cycles
I also think that the check is a bit too strict, since it rejects this:
class C {
a: int
b: int
constructor() { a := 3; b := a; }
Maybe we do want to reject this, but I'm not sure.
…y-lang/dafny into fix-2727-soundness-constant
I did check your example, and it passes. I added it to the current test.
Let me explain in deeper detail the solution I implemented. This solution has almost nothing to do with the cycle dependency checker that is already implemented. This cycle dependency checker builds a dependency graph between constants based on their definition, and find cycles in this graph. Now about the detection of uninitialized recursion: There is a case "Please break this constant initialization cycle", but I don't know how to test it, and it seems from the resolver that this is already being taken care of in previous resolution steps, so I don't think there is a way we could reach this code. I could remove it or throw an error if you prefer. But so that this code is locally consistent and does not require other hypotheses, I would prefer to leave it there. |
I don't understand. When I built your code locally and run it on this:
I get this output:
Do you not see this? |
Excellent point, thanks. How do you deal with conditionals? I'm seeing this with your code: class C {
const a: int;
constructor(c: bool) {
if true {
a := 1;
// Constant not initialized yet. Missing initialization of field a, which needs to be assigned at this point.
} else {
a := 2;
}
}
} Is that expected?
So, should we allow this?
Is it this case? class C {
const a: int
const b: int := a + 1
constructor() { a := b + 1; }
// Constant not initialized yet. Missing initialization of field through the dependency b -> a, which needs to be assigned at this point.
} Sorry if I'm missing something (since it seems that I'm not running the same code as you are based on the example above) |
Here is what I have on my side. So we really have different outputs. That's weird.
For the conditionals, I hadn't realized you could put conditional statements in the first part of the body. Good catch. I'll fix it. |
It's actually for this case:
which, incidentally, runs before the check for recursion of constants. Hence, if I see a variable already, I could also consider that it's initialized already and leave the rest of the resolver to actually report the recursion error. Would that make sense? |
It seems to me that disallowing reading constants could prevent the useful pattern:
Not making it possible to read PREFIX when assigning constants would be cumbersome in this case. We could get around this by assigning PREFIX in the constructor to a common value with FULL_NAME:
but now, it seems that 1) we would be adding a breaking change for something that was legit (the first above), and 2) the workaround breaks the idea that the prefix is instance-independent. |
Should that pattern use a static constant? |
That would work as well. So what about the nested pattern?
This one seems natural: As soon as we defined the constant, we should be able to read it without restriction. |
I don't understand the code above (how can you assign to a static constant?) |
I just fixed my example, there was something wrong. Please check again. Also, I did not realize but until now, we were able to assign a value to constants multiple times. This should not be the case.
With the code on this PR, it will complain that "a" was already assigned. Do you think it is better, or is it expected that you can modify a constant as many times as you want in the first part of a constructor? |
I ended up removing this extra check, as the verifier is clever enough to not assign final values to |
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.
We have decided to debate what the exact semantics of assignment in constructors should be before we merge this. In particular, should constants really behave as functions, like in the following example?
class C {
const a: int
const b := a + a
constructor() {
a := 1;
print b; // 2
a := 2;
print b; // 4
}
Ok, this PR introduces another soundness-crash issue that needs to be fixed Previously, the following would not verify class Dummy {
const i: int
constructor(i: int) {
this.i := i;
}
}
class ThisError {
const d: Dummy
const s: set<ThisError>
constructor() {
s := {this};
// Here it was complaining about the use of "this.s" in the first part of the constructor.
var x := set n: ThisError | n in s :: n.d.i;
d := new Dummy(1);
}
} As this PR has assumed "s" can be used safely, the error is not there anymore. Will need a fix. |
This is actually not a new soundness crash issue, it existed before: |
This PR fixes #2727 by adding an appropriate error message if constants are initialized in the wrong order.
It also fixes #2923
Implementation
CheckInit
phase, I'm keeping track of assignments and statements, andBy submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.