Introduce {:isolate} and {:isolate "paths"} attributes for assert and return commands
#952
Conversation
keyboardDrummer
changed the title
/isolatePaths option and {:isolate_paths} attribute
keyboardDrummer
force-pushed
the
isolatePaths
branch
2 times, most recently
from
0eb1327
to
e3a6d66
Compare
Source/Core/AST/CallCmd.cs
Outdated
| @@ -882,7 +882,7 @@ protected override Cmd ComputeDesugaring(PrintOptions options) | |||
|
|
|||
| // Do this after copying the attributes so it doesn't get overwritten | |||
| if (callId is not null) { | |||
| a.CopyIdWithModificationsFrom(tok, req, | |||
| (a as ICarriesAttributes).CopyIdWithModificationsFrom(tok, req, | |||
There was a problem hiding this comment.
Is this coercion necessary? I seems that AssertCmd always implement this interface, right?
There was a problem hiding this comment.
Refactoring from a previous PR that through a git error was reverted in this PR.
There was a problem hiding this comment.
That issue happened to me in the past as well 😖
|
|
||
| namespace VCGeneration; | ||
|
|
||
| class SplitAttributeHandler { |
There was a problem hiding this comment.
Is this file only refactored from somewhere else?
There was a problem hiding this comment.
It's part of the old ManualSplitFinder.cs, but it's changed quite a bit.
| @@ -1445,7 +1430,7 @@ public static Counterexample AssertCmdToCloneCounterexample(VCGenOptions options | |||
| { | |||
| Contract.Assert(block != null); | |||
| Contract.Assume(block.TransferCmd != null); | |||
| returnCmd = gotoCmdOrigins.GetValueOrDefault(block.TransferCmd); | |||
| returnCmd = block.TransferCmd.tok is GotoFromReturn gotoFromReturn ? gotoFromReturn.Origin : null; | |||
There was a problem hiding this comment.
I now see how you encode meta-data in tokens.
I had launched a discussion about the design of having different tokens vs. flattened tokens here:
dafny-lang/dafny#3354
It all boils down to deciding if you want to encode multiple information using wrapped tokens, where each token provides more information. If that is the case, then a flat token containing everything is more maintainable.
If the cases are truly disjoint, then a hierarchy of tokens is the way to go.
In the light of that, are your new tokens wrapper nestable or not?
There was a problem hiding this comment.
IMO the name Token should be reserved for lexical tokens, so data that only describes a source location and a word. However, I think for error reporting purposes, it is useful to track what the origin of nodes in the AST is, that origin might be ParsedFromSource(IToken token) : IOrigin, but it can also be something related to a program transformation. In this case, the origin of the goto jump is the desugaring transformation of a return jump.
I think in the future I would prefer to rename IToken to IOrigin.
There was a problem hiding this comment.
Agreed that IToken can be renamed to IOrigin, so that the token part is a sub-part of it, concerns only the lexical token, and not every metadata we don't know otherwise where to store.
In the meantime, if there is a token wrapper, I suggest there is at most one such wrapper that will contain any possible information about the origin. Is it what you did? Or are there at least two token wrapper classes?
There was a problem hiding this comment.
Is it what you did? Or are there at least two token wrapper classes?
I think so, if I understand you correctly. Every AST node only has a single origin related field, whether that is a Token or a TokenWrapper.
| @@ -2,10 +2,8 @@ non-interference-2.bpl(25,1): Error: Non-interference check failed | |||
| Execution trace: | |||
| non-interference-2.bpl(7,3): anon0 | |||
| non-interference-2.bpl(12,5): inline$AtomicIncr$0$anon0 | |||
| (0,0): inline$Civl_NoninterferenceChecker_yield_PC$0$L | |||
There was a problem hiding this comment.
I get that (0, 0) is not useful, but what about the label on the right? It looked useful.
There was a problem hiding this comment.
Yeah.. that name is still in the codebase, but someone needs to assign a proper token to the generated node that has that name before it will show up in the execution trace. I can't really judge what the civl code is doing and what tokens should be where.
I can pass in a variant of Token.NoToken, and then it would show this exact message again. I'm not sure it's an improvement though. I'd rather leave this as is.
There was a problem hiding this comment.
Where is the message stored? If it's not in the token, we should display it without token information. If it was in the token, and is lost because of the new encoding, I'd recommend to revert that change.
I worry about breaking Civi users who relied on the last line being printed.
There was a problem hiding this comment.
Okay, I've made it so that nothing changes in the civl tests.
Co-authored-by: Mikaël Mayer <[email protected]>
…r assert and return commands (boogie-org#952)" This reverts commit 25160fa.
Changes
{:isolate}attribute on AssertCmd andReturnCmd{:isolate "paths"}attribute on AssertCmd andReturnCmd/vcsSplitOnEveryAsserttogether with focusTesting
Test/implementationDivisionthat has tests for{:isolate},{:isolate "paths"},{:focus}and{:split_here}