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

Support for Ghost Fields #766

Merged
merged 14 commits into from
May 24, 2024
Merged

Conversation

ArquintL
Copy link
Member

@ArquintL ArquintL commented May 6, 2024

This PR depends on PRs #747 & #755 and, thus, targets ghost-field-target-branch, which corresponds to the branch used in #755.

This PR adds ghost fields, i.e., allows the use of ghost in front of field declarations. Note that struct embeddings can (so far) not be ghost. While ghost fields work mostly analogously to actual, non-ghost, fields, there are a few restrictions / differences to point out:

  • A struct with ghost fields cannot implement an interface, however, a pointer to such a struct can (see ghost-field-interface-simple01.gobra and ghost-field-interface-fail01.gobra)
  • Actual equality (==) on structs considers only actual fields, i.e., structs are equal modulo ghost fields. To include ghost fields in the comparison, ghost equality (===) can be used instead (see ghost-field-comparison-simple01.gobra)
  • As a consequence, a Go map with a struct with ghost fields as a key type becomes challenging as two ghost-unequal key values can be actual-equal and, thus, map to the same entry at runtime. Although our encoding could be extended, we currently disallow such map types (see ghost-field-map-fail01.gobra)

@ArquintL ArquintL requested a review from Felalolf May 6, 2024 15:24
@jcp19
Copy link
Contributor

jcp19 commented May 6, 2024

I will soon test this out in SCION to make sure that this works with the SCION idioms. I will report the results here.

@jcp19
Copy link
Contributor

jcp19 commented May 6, 2024

I just adapted the SCION code to be accepted by this new PR (viperproject/VerifiedSCION#337). As such, I do NOT have any objections against merging this 🚀. Note that this comment is not a review, I did not look at the code.

@ArquintL ArquintL changed the base branch from master to ghost-field-target-branch May 14, 2024 09:06
@ArquintL ArquintL requested a review from jcp19 May 14, 2024 09:07
Copy link
Contributor

@jcp19 jcp19 left a comment

Choose a reason for hiding this comment

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

LGTM! I would be happy to merge this when the tests go through

Copy link
Contributor

@Felalolf Felalolf left a comment

Choose a reason for hiding this comment

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

I think I found an issue. Please look at that. otherwise, it should be fine. I did not check all tests yet.

(underlyingType(typ(s.base)), isGhostField) match {
case (_, true) => isGhost // ghost fields are always ghost memory
val isGhostEmbedding = s.path.exists {
case MemberPath.Next(decl) => decl.ghost
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not super sure whether this is sufficient. Can the path not be longer?

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 don't quite understand. I'm iterating here over the entire field selection's path

Copy link
Contributor

@Felalolf Felalolf left a comment

Choose a reason for hiding this comment

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

missed something

@ArquintL ArquintL changed the base branch from ghost-field-target-branch to tmp-branch-for-#747-#755-#766 May 24, 2024 07:00
@ArquintL ArquintL merged commit aa44146 into tmp-branch-for-#747-#755-#766 May 24, 2024
3 checks passed
@ArquintL ArquintL deleted the ghost-field branch May 24, 2024 07:37
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.

None yet

3 participants