Skip to content
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

Merged
merged 92 commits into from
Sep 4, 2024

Conversation

MikaelMayer
Copy link
Member

Description

This fixes a lot of issues in the Rust compiler, and implements the following features as well:

  • Support for externs and appropriate documentation
  • Compiled subset constraints in quantifiers
  • Eta-expansion of lambdas (Fabio)
  • Reserved names fixups
  • Elephant operator fixup (Siva)
  • Issues with maps in the runtime
  • special field: map.Items
  • Type tests
  • Fix ambiguity in rendering / parsing
  • Full path use for hashing to avoid ambiguity
  • Support for type synonyms
  • Removed of dead code (phantom variants that are useless, upcast for _default classes)
  • Separation Name vs. Var so that escaping can be different and appropriate
  • Paths are now shared between types and expressions
  • Generated code factors some paths in use declarations for debuggability and readability
  • Generation of Rust code by nested modules instead of encoding with "_d" as the separator of modules
  • Interop: Ability to create objects given a sized struct
  • Fixed coercions
  • Tested option for raw pointers with the flag --raw-pointers
  • Documentation for GenTypeContext

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

MikaelMayer and others added 30 commits June 24, 2024 12:46
…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
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
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
Copy link
Member

@robin-aws robin-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the rest!

Copy link
Member

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]
Copy link
Member

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?

Copy link
Member Author

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.

Comment on lines 192 to 195
// print "Left:"
// print _e0;
// print "Right:"
// print _e1;
Copy link
Member

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) &&
Copy link
Member

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.

Copy link
Member Author

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
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member Author

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 {
Copy link
Member

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)

Copy link
Member Author

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.");
Copy link
Member

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"
Copy link
Member

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

Copy link
Member Author

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: #5714

@kbuaaaaaa
Copy link
Contributor

kbuaaaaaa commented Aug 23, 2024

Hi! Sorry to bother but the fuzzers started running when this PR got synced and they all tripped on this:

Code:
Anything
Command:
dafny run/build/translate
Output:
Error: --raw-pointers can only be used after --target:rs or -t:rs

Not sure if this is something you intended to do.

@robin-aws
Copy link
Member

@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?

robin-aws
robin-aws previously approved these changes Aug 23, 2024
Source/DafnyCore/Options/CommonOptionBag.cs Outdated Show resolved Hide resolved
Comment on lines +185 to +186
// 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";
Copy link
Member

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.

Copy link
Member Author

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.

@kbuaaaaaa
Copy link
Contributor

@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?

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.

@keyboardDrummer keyboardDrummer dismissed their stale review August 26, 2024 11:25

PR has been fully reviewed

@MikaelMayer MikaelMayer enabled auto-merge (squash) September 3, 2024 19:58
Copy link
Member

@robin-aws robin-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦀 🎉 🦀 🎉 🦀 🎉 🦀 🎉 🦀

@MikaelMayer MikaelMayer merged commit c9fe1be into master Sep 4, 2024
22 checks passed
@MikaelMayer MikaelMayer deleted the feat-rust-externs-subsets-eta-names-tests branch September 4, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants