-
Notifications
You must be signed in to change notification settings - Fork 271
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
Feat rust externs subsets eta names tests #5613
Feat rust externs subsets eta names tests #5613
Conversation
…onym types and UTF8 char literals.
…ppropriate Fixed support for "this" and constants in reference counting mode. Removed old obsolete class implementations Added all tuples till 16 with a script Support for arrays with 16 dimensions Feature classes and arrays Added documentation about target type Full feat rust Support for Reference counted pointers everywhere. Fixed self type for datatypes vs classes Fixed detection of uninitialized reference counted array Fixed an issue about elseBranch's statements being in the wrong fork. No reading of objects, only borrows Assertion fixed fixup for feat-rust-classes: Nightly crimes no longer printing anything
…, loops & quantifiers, datatype fields ownership, fixed cloning of closed lambda objects, fixed coercion problems Fixup for feat-rust-loops
…names escaped reserved names: {s,S}elf become r#_{s,S}elf rather than r#{s,S}elf + removed them from reserved_rust
Fixup feat-rust-traits expected test fixup traits - no full qualifier for methods if not necessary
…ctions members in datatypes, reserved names - PartialEq, Eq, and Default traits not implemented when datatype contains non-equatable types - note: PartialEq does not have to be manually implemented for parameterized types - note: Auto-generated implementations of Hash constrain type arguments to be subtypes of DafnyType, not DafnyTypeEq - Implementations of fmt_print write "<function>" for members of function type - Implementations of hash ignore members of function type datatypes: style fixes reserved names: destructors containing reserved field names avoid field punning
…ironment closures owned
… memory leaks with MIRI
… function outputs
fixup extern: ExternCompanion fixup externs: Ensure it works for traits as well. Test for the compilation error Fixup external classes test Fixup external classes when no conflict + documentation Ensure order of imports is deterministic and alphabetic Fixup advanced externs and patterns Fixup: no emitting pub use extern if everything is defined. Fixed field change Fixup externs Fixup externs: Ensure the default class decl does not need the extern attribute Fixup externs fixup externs: Classes are extern means their implementation must be provided externally Function externs correctly have their full extern path
… in quantifiers Fixup init value for synonym types
…lambdas functions: more robust eta-expansion
constants: test to make sure constant lambdas are compiled properly (eta-expanded and type-annotated)
…evel and value-level (variables, etc.) names differently, the latter accounting for reserved fields
…les in scope - desugaring of elephant operator now declares and initializes head variable after error handling - as a result, code generation avoids declaring the head variable - the old type system now avoids duplicating the head variable in scope - NB: the new type system does not have this fix yet b/c by changing the declaration order of the head variable, a resolver invariant is broken
…e, conversions, etc. Items and map no longer requires V to have Eq trait
…ithub.com/dafny-lang/dafny into feat-rust-externs-subsets-eta-names-tests
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.
And the rest!
Source/DafnyCore/Backends/SinglePassCodeGenerator/SinglePassCodeGenerator.cs
Show resolved
Hide resolved
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.
(out of scope but top of mind) For the next PR, let's put a banner that says something like "all contents subject to change except for..." and define exactly the scope of symbols non-Dafny generated code is allowed to refer to.
@@ -441,38 +441,6 @@ mod tests { | |||
assert_eq!(Rc::strong_count(&x_copy.data), 1); | |||
} | |||
|
|||
#[test] |
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.
Were these just dropped or moved somewhere else?
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.
Just dropped because we don't wrap classes with box anymore and we won't support it in the near-term.
// print "Left:" | ||
// print _e0; | ||
// print "Right:" | ||
// print _e1; |
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.
nit: should \n as well (the actually implementation does)
EmitExpr(negated, false, guardWriter, wStmts); | ||
EmitHalt(s.Tok, s.Message, bodyWriter); | ||
if ( | ||
Options.Get(CommonOptionBag.Verbose) && |
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 think it would be great to enable this unconditionally, personally. Just getting a source location and "expectation violation" is often very unhelpful, and most tests don't bother with a second argument to expect
to help describe the failure.
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.
As we discussed, I enabled it unconditionally, but only for the Rust backend since printing is not doing its expected job in other backends.
@@ -0,0 +1,38 @@ | |||
// NONUNIFORM: Temporary development of the Rust compiler |
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'd replace these comments with something like "rust-specific tests". Otherwise I can just imagine finding this "Temporary" years later and being just a bit embarrassed. :)
Nothing wrong with having some tests written for specific backends - we still have c++
tests after all.
Although we should also follow up with running these tests on all backends with %testDafnyForEachCompiler
soon, as that will check that Rust behaves consistently. We can run all rust tests with and without --raw-pointers
just as we run all C# tests with and without --include-runtime
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.
Just on top of my mind, it occurs to me that, whenever we run a Rust test, I believe it fetches all dependencies directly from the internet. The target folder weights 1.5Gb every time. This worries me for the CI capacity but I don't know if we can do something about it.
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 probably don't want to run tests with the two possible option values for --raw-pointers
, because most tests don't have pointers, so that would be wasting CI resources.
But I'll replace temporary with something more appropriate
} | ||
} | ||
|
||
module Dafny.FileIO { |
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.
If this implementation is working, why not move it to the actual DafnyStandardLibraries?? (Can be in a follow-up PR for sure, but worth prioritizing)
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 is only a mock implementation of Dafny.FileIO. The extern actually don't read the file system. But it gives an idea on how to achieve that. Let's think of a proper standard library in an upcoming PR.
pub mod FileIO { | ||
pub fn INTERNAL_ReadBytesFromFile(path: &::dafny_runtime::DafnyString) | ||
-> ::dafny_runtime::DafnyString { | ||
return ::dafny_runtime::string_of("Everything is ok."); |
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.
Ah damn, never mind :) (would still love to fill in the real thing ASAP though)
// NONUNIFORM: Temporary development of the Rust compiler | ||
// RUN: %baredafny run --target=rs "%s" > "%t" | ||
// RUN: %diff "%s.expect" "%t" | ||
// RUN: %OutputCheck --file-to-check "%S/nestedmodules-rust/src/nestedmodules.rs" "%S/nestedmodules.check" |
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.
Deserves a comment as to what the CHECK pattern is checking
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.
// Checks that, despite module B being prefixed with A, there will be a "mod B" somewhere
// and not an encoding like "mod A_B".
// This module will automatically be in mod A and referred to as "A::B"
@@ -0,0 +1,3 @@ | |||
|
|||
Dafny program verifier finished with 2 verified, 0 errors | |||
(0,-1): Error: Microsoft.Dafny.UnsupportedInvalidOperationException: Creation of object of type Test<Positive> requires a constructor |
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.
FYI: #5714
Hi! Sorry to bother but the fuzzers started running when this PR got synced and they all tripped on this: Code: Not sure if this is something you intended to do. |
@kbuaaaaaa Thanks for the heads up! As it happens the CI caught this too just fine. Would it be worth having the fuzzer integration only run when the CI for a PR is green first, or at least check the CI results if fuzzing fails? |
// For now, this code prints nicely only in the Rust code generator until we make it work for every code generator | ||
var specialExpectEqualHandling = Options.Backend.TargetId == "rs"; |
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.
Happy with this for now, but can you cut an issue linked to #5561 to track this? At a minimum I'd like to tweak the output format a little too.
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.
Here is the issue.
#5720
However, I'm not sure I should track it to the Dafny-to-Rust code generator since it's not related.
Thank you for the feedback! Just checked the CI and you’re right. It is definitely more resource efficient to wait for the CI first before fuzzing in this case. |
Co-authored-by: Robin Salkeld <[email protected]>
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.
🦀 🎉 🦀 🎉 🦀 🎉 🦀 🎉 🦀
Description
This fixes a lot of issues in the Rust compiler, and implements the following features as well:
--raw-pointers
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.