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

Fix issue 491 #531

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Fix issue 491 #531

wants to merge 16 commits into from

Conversation

Felalolf
Copy link
Contributor

This PR:

  • Fixes a bug in the encoding context, where using the copy constructor would cause the freshname generator to break.
  • Makes the length argument of array type expressions constant. This makes our treatment of types more consistent.
  • Fixes an unsoundness with length.
  • Fixes an unsoundness where panics that are caused by referencing L-values were not detected. Instead of checking that every L-value is safe immediately, we check that all usages of L-values are safe. See safeReference for more details.
  • Fixes several unsoundnesses in the encoding of zero sized types.
  • Adds an UncheckedRef node. The node is necessary to make our encoding modular.
  • Renames InterfaceReceiverIsNilReason to the more general ReceiverIsNilReason.
  • Fixes some error codes. Previously, assignments and dereferencs sometimes failed with exhale and call error, respectively, and now fail with assignment and deref error, respectively.

There is one failing test. The test fails due to an error in the desugaring of closures. I will most likely add an IgnoreFile annotation and open a separate issue. The currently generated internal representation should fail since it contains an unsafe dereference.

}

@tailrec
private def cannotBeNil(l: in.Expr): Boolean = l match {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to only check locations? If so, I would probably make the param type more restrictive (i.e., in.Location)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it also checks expressions (in particular, see the body of the method, where we use it to check whether the receiver of an index and field-ref expression cannot be nil).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could change it to something along the lines of:

private def cannotBeNil(l: in.Location): Boolean = {
  def aux(e: in.Expr): Boolean = {
    // body of cannotBeNil
  }
  aux(l)
}

@Felalolf
Copy link
Contributor Author

Felalolf commented Oct 3, 2022

This PR still has issues because of which it is not merged

@jcp19 jcp19 linked an issue Oct 11, 2023 that may be closed by this pull request
@jcp19
Copy link
Contributor

jcp19 commented Jul 17, 2024

This PR still has issues because of which it is not merged

For reference, the issue mentioned here is that Gobra is often not able to prove non-nilness of memory locations that are obviously non-nil. Consider the simple example below

requires  8 <= len(raw)
preserves forall i int :: { &raw[i] } 0 <= i && i < len(raw) ==>
	acc(&raw[i])
func DecodeFromBytes(raw []byte) {
	assert forall i int :: 0 <= i && i < len(raw[2:4]) ==>
		&raw[2:4][i] == &raw[2 + i]
}

In the current status of the PR, we get the following error:

[info] Error at: </Users/joao/000491-bug.gobra:7:58> Reading might fail. 
[info] The receiver raw[2:4][i] might be nil

@jcp19 jcp19 self-requested a review July 18, 2024 12:17
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.

Dereferencing feature
2 participants