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

It's unsound to refine the operand of an instanceof. #735

Merged
merged 5 commits into from
May 31, 2016
Merged

Conversation

smillst
Copy link
Member

@smillst smillst commented May 20, 2016

However, the operand of an instance of is non-null if the instanceof is true.

This changes cause a new (legitimate) Daikon type-checking error at this line. It's a method.invocation.invaild error on pslice.num_samples();.

smillst added 2 commits May 19, 2016 10:12
However, the operand of an instance of is non-null if the instanceof is true
@smillst smillst assigned wmdietl and smillst and unassigned wmdietl May 20, 2016
@smillst
Copy link
Member Author

smillst commented May 24, 2016

Add testcase

@smillst smillst assigned wmdietl and unassigned smillst May 24, 2016
@smillst smillst assigned smillst and unassigned wmdietl May 25, 2016
@smillst smillst assigned wmdietl and unassigned smillst May 25, 2016
@mernst
Copy link
Member

mernst commented May 26, 2016

I resolved the Daikon type-checking issue by adding annotations to Daikon indicating that num_samples can be called even when its receiver is under initialization.

@smillst
Copy link
Member Author

smillst commented May 26, 2016

@wmdietl I would like to rebase this pull request into one commit before merging

@wmdietl
Copy link
Member

wmdietl commented May 27, 2016

Is it unsound for type systems other than Nullness because the property is not checked at runtime?

If that's the reasoning, shouldn't the same be done for casts? A cast without runtime check is equally unsound.

@wmdietl wmdietl assigned smillst and unassigned wmdietl May 27, 2016
@smillst
Copy link
Member Author

smillst commented May 27, 2016

@wmdietl is this pull request ok to merge?

@smillst
Copy link
Member Author

smillst commented May 27, 2016

Is it unsound for type systems other than Nullness because the property is not checked at runtime?

Yes

If that's the reasoning, shouldn't the same be done for casts? A cast without runtime check is equally unsound.

Correct, which is why a warning is issued if the cast is unsafe.

@smillst smillst assigned wmdietl and unassigned smillst May 27, 2016
@wmdietl
Copy link
Member

wmdietl commented May 27, 2016

If that's the reasoning, shouldn't the same be done for casts? A cast without runtime check is equally unsound.

Correct, which is why a warning is issued if the cast is unsafe.

So why not do the same thing for instanceof? Refine the type and issue a warning. The two seem equally unsound, so why treat them differently?

@wmdietl wmdietl assigned smillst and unassigned wmdietl May 27, 2016
@smillst
Copy link
Member Author

smillst commented May 31, 2016

So why not do the same thing for instanceof? Refine the type and issue a warning. The two seem equally unsound, so why treat them differently?

We can do this, and I've opened an issue for this request, #762. However, we can't issue the warning without defaulting the reference type of the instanceof to top and it is not straight forward to do so. I would like to merge this pull request so that this fix appears in the release.

@smillst smillst assigned wmdietl and unassigned smillst May 31, 2016
@wmdietl
Copy link
Member

wmdietl commented May 31, 2016

That sounds like a good plan. I would add a reference from #762 back to this change, to have the basic implementation of the refinement feature.

@wmdietl wmdietl assigned smillst and unassigned wmdietl May 31, 2016
@smillst smillst merged commit f1a3bc2 into master May 31, 2016
@smillst smillst deleted the instanceof branch May 31, 2016 20:02
Ao-senXiong pushed a commit to opprop/checker-framework that referenced this pull request Jul 18, 2024
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.

3 participants