-
Notifications
You must be signed in to change notification settings - Fork 259
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
Pure parse function #4083
Pure parse function #4083
Conversation
b6c457c
to
05bb8ff
Compare
05bb8ff
to
81c8cec
Compare
} | ||
public UserDefinedType ArrayType(IToken tok, int dims, List<Type> optTypeArgs, bool allowCreationOfNewClass, bool useClassNameType = false) { | ||
|
||
public static (UserDefinedType type, Action<BuiltIns> ModifyBuiltins) ArrayType(IToken tok, int dims, List<Type> optTypeArgs, bool allowCreationOfNewClass, bool useClassNameType = false) { |
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.
Can we document the difference between this ArrayType
and the previous one?
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.
There is some implicit documentation in the fact that this one is static. I'll add documentation to the non-static one saying it updates the instance.
var eeCopy = ee; | ||
BuiltinsModifiers.Add(b => b.ArrayType(eeCopy.Count, new IntType(), true)); |
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.
Do we need a full copy or just the count?
var eeCopy = ee; | |
BuiltinsModifiers.Add(b => b.ArrayType(eeCopy.Count, new IntType(), true)); | |
var eeCount = ee.Count; // ee might be modified later, but we want the original array dims | |
BuiltinsModifiers.Add(b => b.ArrayType(eeCount, new IntType(), true)); |
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 eventual list size determines the required array type. The list size shouldn't be edited from this point on, unless ArrayType is called again with the new size, so I think either is fine.
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 large can BuiltinsModifiers
get, and could it be a performance concern that we store all these lambdas instead of modifying the BuiltIns
immediately?
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.
It'll be much smaller than the parsed AST, so I don't think there is a concern. We could dedupe the list, but I feel right now that would be premature optimization.
BuiltIns.ArrayType(Token.NoToken, dims, | ||
new List<Type> { new InferredTypeProxy() }, true).ModifyBuiltins(resolver.builtIns); |
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 might be missing something, but how does this compile when BuiltIns.ArrayType(IToken, int, List<Type>, bool, bool = false)
returns a tuple?
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.
It returns a tuple with named members, and .ModifyBuiltins
accesses one those, similar to what .ItemX
would do.
// TODO move this out of ErrorReporter together with OuterModule. Let Program have a list of FileModuleDefinitions as constructor input and create its default module there. | ||
if (tok.FromIncludeDirective(OuterModule) && OuterModule != null) { | ||
var include = OuterModule.Includes.First(i => i.IncludedFilename == tok.Uri); | ||
if (!include.ErrorReported) { | ||
Message(source, ErrorLevel.Error, null, include.tok, "the included file " + Path.GetFileName(tok.ActualFilename) + " contains error(s)"); | ||
include.ErrorReported = true; | ||
} | ||
} |
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'm all for this change, but we should be sure to mention this in the CHANGELOG since I know there are users (including myself) who are accustomed to the behavior, and might be confused if we change it silently.
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 left a few comments above, none blocking. Thanks for making this one easier to review.
### Changes - Let the Dafny language server use a cache for parsing. The cache stores items as long as they were used in the previous compilation. This means that commenting out an include can cause a large piece of cache to be pruned, leading to a possibly unexpected slowdown when it's uncommented again. ### Caveats I've decided not to add an option to turn off caching, since I don't see the use-case. Of course caching is a computation vs storage trade-off, and we could provide many options to configure the caching behavior, but I think the current caching is cheap enough (at most storing two sets of parse results at a time) that there is no reason to want to turn it off. Please carefully review the code that determines the cache key, since a bug in that would lead to outdated compilation results. ### Testing - Add a test that checks if parsing is cached, and that the cache prunes entries when they're not hit. - Add test for included file has error message for indirect includes, something that was still missing from #4083 <small>By submitting this pull request, I confirm that my contribution is made under the terms of the [MIT license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>
Follow-up of #4059
NOTE: please review the first 4 commits separately from the rest, since they contain code moves and renames.
Changes
ParseFile.ParseFile
take and modify an ErrorReporter, but instead let it return one.Builtins
, but instead let it return a list of actions that can modify aBuiltin
.Document
into their own fileInclude
class into its own file.Testing
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.