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

Verify The Jackbot 10684 #251

Merged
merged 8 commits into from Nov 26, 2020

Conversation

kammola
Copy link
Contributor

@kammola kammola commented Nov 18, 2020

No description provided.

@kammola
Copy link
Contributor Author

kammola commented Nov 18, 2020

@vakaras

@kammola kammola marked this pull request as draft November 18, 2020 14:19
@kammola kammola marked this pull request as ready for review November 18, 2020 14:20
@fpoli
Copy link
Member

fpoli commented Nov 18, 2020

Could you remove the .DS_Store files from the PR?

@kammola
Copy link
Contributor Author

kammola commented Nov 18, 2020

Could you remove the .DS_Store files from the PR?

Done.

@vakaras
Copy link
Contributor

vakaras commented Nov 18, 2020

@kammola The tests are failing because on CI we test with the overflow-checks enabled while IDE by default has them disabled. Here are the instructions on how to enable the checks in the IDE.

#[requires (i >= 0)]
#[requires (i < seq.len())]
#[requires (seq.len() > 0)]
fn solve_rec(seq: &VecWrapperI32, i: usize) -> i32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say that it is not obvious that this function is computing the result we want it to compute. Therefore, we need to prove that this is indeed the case. One idea would be to do this as follows:

  1. Write a function solve_naive(seq: &VecWrapperI32) -> i32 that solves the problem by using two nested loops (one loop checks all possible start positions of the subsequence, and the second one computes all possible sums that start at that position).
  2. Prove that solve_rec(seq, seq.len()) >= solve_naive(seq) (it should be much easier to prove >= than ==).

Is it clear how to do that?

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 will try to do it and see if I manage.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you get any questions, please let me know.

@kammola
Copy link
Contributor Author

kammola commented Nov 18, 2020 via email

@vakaras
Copy link
Contributor

vakaras commented Nov 18, 2020

I have changed it but I am facing overflow problems now. Is there a way to avoid that or should I also verify the overflow won’t happen?

I think the final version should be verified to be overflow free. Please let me know if you get problems with this.

@kammola
Copy link
Contributor Author

kammola commented Nov 18, 2020

I have changed it but I am facing overflow problems now. Is there a way to avoid that or should I also verify the overflow won’t happen?

I think the final version should be verified to be overflow free. Please let me know if you get problems with this.

The final version gives me overflow problems

@vakaras
Copy link
Contributor

vakaras commented Nov 18, 2020

I have changed it but I am facing overflow problems now. Is there a way to avoid that or should I also verify the overflow won’t happen?

I think the final version should be verified to be overflow free. Please let me know if you get problems with this.

The final version gives me overflow problems

The task description says that each number is not larger than 1000. Adding this to the precondition and propagating down should allow convincing Prusti that there are no overflows.

@kammola
Copy link
Contributor Author

kammola commented Nov 18, 2020

I have changed it but I am facing overflow problems now. Is there a way to avoid that or should I also verify the overflow won’t happen?

I think the final version should be verified to be overflow free. Please let me know if you get problems with this.

The final version gives me overflow problems

The task description says that each number is not larger than 1000. Adding this to the precondition and propagating down should allow convincing Prusti that there are no overflows.

I am trying to add this condition #[ensures (result >= -1000 && result <= i * 10000)] , but i is usize and result is i32. it. gives me the error expected i32 found usize. is there a way to do the multiplication in the condition?

@fpoli
Copy link
Member

fpoli commented Nov 20, 2020

I am trying to add this condition #[ensures (result >= -1000 && result <= i * 10000)] , but i is usize and result is i32. it. gives me the error expected i32 found usize. is there a way to do the multiplication in the condition?

Have you tried #[ensures (result >= -1000 && (result as u128) <= (i * 10000 as u128))]?

@kammola
Copy link
Contributor Author

kammola commented Nov 20, 2020

I am trying to add this condition #[ensures (result >= -1000 && result <= i * 10000)] , but i is usize and result is i32. it. gives me the error expected i32 found usize. is there a way to do the multiplication in the condition?

Have you tried #[ensures (result >= -1000 && (result as u128) <= (i * 10000 as u128))]?

Prusti complains without reporting the error. It gives an error at the beginning of the file whenever I use casting.

@fpoli
Copy link
Member

fpoli commented Nov 20, 2020

Prusti complains without reporting the error. It gives an error at the beginning of the file whenever I use casting.

Uh, thanks for reporting that. PR #253 should fix the error reporting.

We actually don't support casting between usize and other integer types, because Rust gives no guarantee about the range of usize. I didn't remember that. The issue #252 that you opened mentions the proper solution.

@vakaras
Copy link
Contributor

vakaras commented Nov 21, 2020

@kammola x as usize should work now if you compile Prusti from source code. (You can find the instructions in the developer guide.)

@vakaras
Copy link
Contributor

vakaras commented Nov 22, 2020

@kammola I have added instructions on how to use a locally built version in Prusti Assistant: https://viperproject.github.io/prusti-dev/dev-guide/development/setup.html#using-locally-built-version-in-prusti-assistant

Comment on lines 4 to 37
pub struct VecWrapperI32 {
v: Vec<i32>,
}

impl VecWrapperI32 {
#[trusted]
#[pure]
#[ensures (0 <= result)]
pub fn len(&self) -> usize {
self.v.len()
}

#[trusted]
#[ensures (result.len() == 0)]
pub fn new() -> Self {
VecWrapperI32 { v: Vec::new() }
}

#[trusted]
#[pure]
#[requires (0 <= index && index < self.len())]
pub fn lookup(&self, index: usize) -> i32 {
self.v[index]
}

#[trusted]
#[ensures (self.len() == old(self.len()) + 1)]
#[ensures (self.lookup(old(self.len())) == value)]
#[ensures (forall(|i: usize| (0 <= i && i < old(self.len())) ==>
self.lookup(i) == old(self.lookup(i))))]
pub fn push(&mut self, value: i32) {
self.v.push(value);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this work with an #[extern_spec] implementation, like here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does #[extern_spec] allow adding a pure lookup function? (As far as I remember there were some serious problems with supporting the Index and IndexMut traits.)

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 tried but I also couldn't find a way to support lookup.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, you're right, the encoding panics when trying to call such a function. I suppose you could write a trusted wrapper only for the lookup function but then it's more messy.

Copy link
Contributor

@vakaras vakaras left a comment

Choose a reason for hiding this comment

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

LGTM

@vakaras vakaras merged commit 445df8f into viperproject:master Nov 26, 2020
@Aurel300
Copy link
Member

Only noticed this now unfortunately; there is a typo in the folder name, should be "competitive".

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

4 participants