-
Notifications
You must be signed in to change notification settings - Fork 54
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
Delete -naive flag and disallow lookup actions in rules #461
base: main
Are you sure you want to change the base?
Changes from all commits
74999fb
3e55b0d
82994eb
dc69b30
dee21e8
9e10fac
35e8532
a593155
dc42cd3
2573722
c484c92
204dd5c
6c8cfbc
3ff74c7
3efe333
94a64c0
a18063e
2abdd10
a040ce9
63fcbff
4cdeebe
59334fb
de7a783
dbbf4c8
4e34bc6
bc92bad
ce7294b
d8378a7
79781cc
30fdddd
693ff70
b6a2466
14aa00d
a9c9969
6c315eb
97ecc71
181cd97
0c0af7a
755cf92
078feb0
34a094c
bb09c17
ead38d8
a3a7f3c
645d3e3
b3768b3
5b95c79
fd20ebf
ac48f48
8a75e7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -388,13 +388,72 @@ impl TypeInfo { | |
let body: Vec<ResolvedFact> = assignment.annotate_facts(&mapped_query, self); | ||
let actions: ResolvedActions = assignment.annotate_actions(&mapped_action, self)?; | ||
|
||
Self::check_lookup_actions(&actions)?; | ||
|
||
Ok(ResolvedRule { | ||
span: span.clone(), | ||
body, | ||
head: actions, | ||
}) | ||
} | ||
|
||
fn check_lookup_expr(expr: &GenericExpr<ResolvedCall, ResolvedVar>) -> Result<(), TypeError> { | ||
match expr { | ||
GenericExpr::Call(span, head, args) => { | ||
match head { | ||
ResolvedCall::Func(t) => { | ||
// Only allowed to lookup constructor | ||
if !t.is_datatype { | ||
Err(TypeError::LookupInRuleDisallowed( | ||
head.to_symbol(), | ||
span.clone(), | ||
)) | ||
} else { | ||
Ok(()) | ||
} | ||
} | ||
ResolvedCall::Primitive(_) => Ok(()), | ||
}?; | ||
for arg in args.iter() { | ||
Self::check_lookup_expr(arg)? | ||
} | ||
Ok(()) | ||
} | ||
_ => Ok(()), | ||
} | ||
} | ||
|
||
fn check_lookup_actions(actions: &ResolvedActions) -> Result<(), TypeError> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could use |
||
for action in actions.iter() { | ||
match action { | ||
GenericAction::Let(_, _, rhs) => Self::check_lookup_expr(rhs), | ||
GenericAction::Set(_, _, args, rhs) => { | ||
for arg in args.iter() { | ||
Self::check_lookup_expr(arg)? | ||
} | ||
Self::check_lookup_expr(rhs) | ||
} | ||
GenericAction::Union(_, lhs, rhs) => { | ||
Self::check_lookup_expr(lhs)?; | ||
Self::check_lookup_expr(rhs) | ||
} | ||
GenericAction::Change(_, _, _, args) => { | ||
for arg in args.iter() { | ||
Self::check_lookup_expr(arg)? | ||
} | ||
Ok(()) | ||
} | ||
GenericAction::Extract(_, expr, variants) => { | ||
Self::check_lookup_expr(expr)?; | ||
Self::check_lookup_expr(variants) | ||
} | ||
GenericAction::Panic(..) => Ok(()), | ||
GenericAction::Expr(_, expr) => Self::check_lookup_expr(expr), | ||
}? | ||
} | ||
Ok(()) | ||
} | ||
|
||
fn typecheck_facts( | ||
&self, | ||
symbol_gen: &mut SymbolGen, | ||
|
@@ -516,6 +575,8 @@ pub enum TypeError { | |
InferenceFailure(Expr), | ||
#[error("{1}\nVariable {0} was already defined")] | ||
AlreadyDefined(Symbol, Span), | ||
#[error("{1}\nValue lookup of non-constructor function {0} in rule is disallowed.")] | ||
LookupInRuleDisallowed(Symbol, Span), | ||
#[error("All alternative definitions considered failed\n{}", .0.iter().map(|e| format!(" {e}\n")).collect::<Vec<_>>().join(""))] | ||
AllAlternativeFailed(Vec<TypeError>), | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,21 +20,21 @@ | |
(relation neq (Math Math)) | ||
|
||
(rule ((neq x y)) | ||
((neq y x))) | ||
((set (neq y x) ()))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see, now relations have to be set. My opinion is that we shouldn't make users use I think the long-term plan is for us to desugar relations to be constructors so people don't have to do this. You could either implement that plan in this PR, or desugar relations to a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I'll implement the desugaring in this PR. My impression is that it is still up to discussion what it should be desugared into. Performance overhead aside, desugaring into a constructor might not be ideal because that makes it more expressive than needed. For instance, now you can union two edges in the relation or have a relation as the codomain of a function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point- we would have to disallow unioning two relation terms. Relation desugaring aside, I don't think we want to make this breaking change when we plan to fix it right away. I guess that means this PR is blocked by solving the relation issue There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make functions whose output is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that would work. I think the plan is to get rid of default values altogether. Desugared or not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant getting rid of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Conclusions:
|
||
|
||
(rule ((neq x x)) | ||
((panic "query (neq x x) found something equal to itself"))) | ||
|
||
|
||
; injectivity rules take not equal to not equal. | ||
(rule ((neq x y) (= (add x z) e)) | ||
((neq (add x z) (add y z)))) | ||
((set (neq (add x z) (add y z)) ()))) | ||
(rule ((= (add x (Num i)) e) (!= i 0)) | ||
((neq e x))) | ||
((set (neq e x) ()))) | ||
|
||
|
||
(rule ((= (Num a) n1) (= (Num b) n2) (!= a b)) | ||
((neq n1 n2))) | ||
((set (neq n1 n2) ()))) | ||
|
||
; select gets from store | ||
(rewrite (select (store mem i e) i) e) | ||
|
@@ -59,9 +59,9 @@ | |
(let r3 (Var "r3")) | ||
(let mem1 (AVar "mem1")) | ||
|
||
(neq r1 r2) | ||
(neq r2 r3) | ||
(neq r1 r3) | ||
(set (neq r1 r2) ()) | ||
(set (neq r2 r3) ()) | ||
(set (neq r1 r3) ()) | ||
(let test1 (select (store mem1 r1 (Num 42)) r1)) | ||
(let test2 (select (store mem1 r1 (Num 42)) (add r1 (Num 17)))) | ||
(let test3 (select (store (store mem1 (add r1 r2) (Num 1)) (add r2 r1) (Num 2)) (add r1 r3))) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we still want to keep the
-naive
flag as well as the code here, so the user can still do naive evaluation (useful for debugging, also have a different semantics than semi-naive for "unsafe" egglog).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree that we should probably keep naive evaluation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should keep it. It is unhelpful and adds complexity to the later passes as they need to support the naive semantics correctly. I am also against keeping it as a use-at-your-own-risk feature.
For Egglog users, if you don't use delete, semi-naive and naive are indistinguishable, so it is unhelpful for debugging. If you use delete, then you care much about performance, and there's no point in using naive. Even when you debug with unsafe features, you should probably debug the semi-naive case instead because that's what you want.
For Egglog developers, I see some value in being a sanity check for ensuring semi-naive is implemented correctly. But we are not doing this now, and it can also be done through stronger end-to-end test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was convincing to me. I've never personally used it before
@yihozhang what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What complexity does this add to later passes? I thought the only difference between seminaive and naive is that in the seminaive case, we split the original query into many small queries depending on the timestamps (i.e., what this code snippet does).
I strongly recommend that we keep the naive evaluation. We can view semi-naive as an optimization of the naive evaluation, and this optimization is not always semantic-preserving, when given bizarre programs that violate certain assumptions. Examples include
extract
/ user-defined primitivesI'm also not confident that our semi-naive is implemented correctly- do we really update timestamp every time we update the table? I just looked at table.rs and it seems we don't update the timestamp for at least
get_mut
. The naive evaluation serves as a ground truth for this purpose. Personally, when I am debugging a primitive I wrote, the first thing I do is to disable semi-naive evaluation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep the naive flag, either we need to split the latter passes into two, which is unlikely, or each piece of downstream code must support both naive and semi-naive. I am skeptical about the claim that semi-naive code would just work for naive. For one thing, I don't see how semi-naive can be implemented as pure syntactic rewrites. As you pointed out, something more needs to happen to the timestamps in the semi-naive case. And yet, the naive flag is not used anywhere else in the codebase.
There are cases where the two give different semantics. However, the naive semantics is not more helpful to the users in those cases because they still need semi-naive to work in the end.
For your last point: Firstly, you still need to debug your new primitive for semi-naive. Secondly, I will only trust naive evaluation as a ground truth if it is well supported with a clear separation between the two semantics. Relying on your program to be tested to produce the test output is a terrible idea to me.
However, I do think this discussion raised a significant concern about the correctness of Egglog. We should investigate the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conclusion: Keep
-naive
for a smaller core-naive
, we settle for the timestamp hack