-
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
Split function
into constructor
/relation
/(custom)function
; Remove default
; Disallow function
lookup in the RHS of a rule
#461
Open
FTRobbin
wants to merge
149
commits into
main
Choose a base branch
from
haobinni-0904
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,317
−1,006
Open
Changes from 9 commits
Commits
Show all changes
149 commits
Select commit
Hold shift + click to select a range
74999fb
Get rid of semi-naive flag
FTRobbin 3e55b0d
Global lookup tests
FTRobbin 82994eb
Merge branch 'main' of github.com:egraphs-good/egglog into haobinni-0904
FTRobbin dc69b30
Add fail corner case to remove_global
FTRobbin dee21e8
Starting to rewrite tests
FTRobbin 9e10fac
Merge branch 'main' of github.com:egraphs-good/egglog into haobinni-0904
FTRobbin 35e8532
Rewrote all failed tests
FTRobbin a593155
Minor
FTRobbin dc42cd3
Minor
FTRobbin 2573722
Revert "Rewrote all failed tests"
FTRobbin c484c92
Merge branch 'main' of github.com:egraphs-good/egglog into haobinni-0904
FTRobbin 204dd5c
New typechecking pass forbidding lookups
FTRobbin 6c8cfbc
Merge branch 'main' of github.com:egraphs-good/egglog into haobinni-0904
FTRobbin 3ff74c7
Fix array.egg
FTRobbin 3efe333
Fix combined_nested.egg
FTRobbin 94a64c0
Fix cykjson.egg
FTRobbin a18063e
Fix cyk.egg
FTRobbin 2abdd10
Revert previous fixes to tests
FTRobbin a040ce9
Fixing eggcc-extraction.egg in progress
FTRobbin 63fcbff
Fix eggcc-extraction.egg
FTRobbin 4cdeebe
Fix fusion.egg
FTRobbin 59334fb
FIx herbie.egg
FTRobbin de7a783
Fix herbie-tutorial.egg
FTRobbin dbbf4c8
Fix path.egg
FTRobbin 4e34bc6
Fix integer_math.egg
FTRobbin bc92bad
Fix interval.egg
FTRobbin ce7294b
Fix list.egg
FTRobbin d8378a7
Fix math.egg
FTRobbin 79781cc
Fix path-union.egg
FTRobbin 30fdddd
Fix pathproof.egg
FTRobbin 693ff70
Fix points-to.egg
FTRobbin b6a2466
Fix prims.egg
FTRobbin 14aa00d
Fix python_arry_optimize.egg
FTRobbin a9c9969
Fix repro-desugar-143.egg
FTRobbin 6c315eb
Fix python_array_optimize.egg again
FTRobbin 97ecc71
Fix repro-querybug.egg
FTRobbin 181cd97
Fix repro-unsound.egg
FTRobbin 0c0af7a
Fix rw-analysis.egg
FTRobbin 755cf92
Fix schedule-demo.egg
FTRobbin 078feb0
Fix stratified.egg
FTRobbin 34a094c
Fix stresstest_large_expr.egg
FTRobbin bb09c17
Fix test-combined.egg
FTRobbin ead38d8
Fix test-combined-steps.egg
FTRobbin a3a7f3c
Fix tricky-type-checking.egg
FTRobbin 645d3e3
Fix typeinfer.egg
FTRobbin b3768b3
Fix until.egg
FTRobbin 5b95c79
Add negative cases for new typechecking pass
FTRobbin fd20ebf
Add more negative cases
FTRobbin ac48f48
Add negative and positive cases for rewrite
FTRobbin 8a75e7e
Minor
FTRobbin b26e01c
Merge branch 'main' of github.com:egraphs-good/egglog into haobinni-0904
FTRobbin 82d3e09
Removed default
FTRobbin 10492d2
Fix bool.egg
FTRobbin 5bf49e4
Revert "Fix bool.egg"
FTRobbin 6fae540
Revert "Fix until.egg"
FTRobbin e33c1c5
Revert "Fix typeinfer.egg"
FTRobbin 0c0fc47
Revert "Fix tricky-type-checking.egg"
FTRobbin cf55a46
Revert "Fix test-combined-steps.egg"
FTRobbin 844658e
Revert "Fix test-combined.egg"
FTRobbin 72b8193
Revert "Fix stresstest_large_expr.egg"
FTRobbin 5b3927b
Revert "Fix stratified.egg"
FTRobbin beaa323
Revert "Fix schedule-demo.egg"
FTRobbin 00f2b79
Revert "Fix rw-analysis.egg"
FTRobbin 6ab6f46
Revert "Fix repro-unsound.egg"
FTRobbin 52d9b87
Revert "Fix repro-querybug.egg"
FTRobbin 3e3d732
Revert "Fix python_array_optimize.egg again"
FTRobbin b4b2299
Revert "Fix repro-desugar-143.egg"
FTRobbin d8623fe
Revert "Fix python_arry_optimize.egg"
FTRobbin 3012cfc
Revert "Fix prims.egg"
FTRobbin fd14c3b
Revert "Fix points-to.egg"
FTRobbin 7c06b1f
Revert "Fix pathproof.egg"
FTRobbin 02834f6
Revert "Fix path-union.egg"
FTRobbin 2d6acc3
Revert "Fix math.egg"
FTRobbin 90e09fb
Revert "Fix list.egg"
FTRobbin 1032ac1
Revert "Fix interval.egg"
FTRobbin 3c81fba
Revert "Fix integer_math.egg"
FTRobbin 0588a41
Revert "Fix path.egg"
FTRobbin 175d337
Revert "Fix herbie-tutorial.egg"
FTRobbin c617979
Revert "FIx herbie.egg"
FTRobbin 0369878
Revert "Fix fusion.egg"
FTRobbin 9b37f06
Revert "Fix eggcc-extraction.egg"
FTRobbin 546fbdc
Revert "Fixing eggcc-extraction.egg in progress"
FTRobbin 0e72f84
Revert "Fix cyk.egg"
FTRobbin bd586c8
Revert "Fix cykjson.egg"
FTRobbin 85d101d
Revert "Fix combined_nested.egg"
FTRobbin 9376216
Revert "Fix array.egg"
FTRobbin 4d307d0
Revert "Get rid of semi-naive flag"
FTRobbin bf4133d
Fix desugar.rs
FTRobbin f95798d
Switching to Constructor/Relation/Custom subtypes
FTRobbin 6e47d0e
Minor
FTRobbin 799554e
Fix remove_globals.rs
FTRobbin 22c8010
Fix antiunify.egg
FTRobbin 153e9cd
Fix array.egg
FTRobbin e4b1910
Fix bdd.egg
FTRobbin c6b81ca
Fix calc.egg
FTRobbin d8387e0
Fix combinators.egg
FTRobbin e98ac47
Added combinators_function.egg
FTRobbin efb2481
Fix container-rebuild.egg
FTRobbin 3b1432b
Fix cyk.egg
FTRobbin aed3564
Fix cykjson.egg
FTRobbin 7469e3e
Fix eggcc-extraction.egg
FTRobbin 0884a62
Fix eqsat-basic-multiset.egg
FTRobbin 5c12556
Fix fibonacci-demand.egg
FTRobbin b65b862
Fix fusion.egg
FTRobbin 097b1aa
Fix herbie-tutorial.egg
FTRobbin d051a35
Fix intersection.egg
FTRobbin 8ba1fcf
Fix interval.egg
FTRobbin 0b7bee0
Fix knapsack.egg
FTRobbin fe42a86
Fix lambda.egg
FTRobbin 8082f83
Fix levenshtein-distance.egg
FTRobbin d886d36
Fix list.egg
FTRobbin b01d1d4
Fix matrix.egg
FTRobbin 4a39a28
Recreated merge-saturates.egg
FTRobbin 0d1d358
Fix multiset.egg
FTRobbin adc0893
Fix python_array_optimize.egg
FTRobbin 374ad58
Fix repro-define.egg
FTRobbin 3d62de9
Fix repro-desugar-143.egg
FTRobbin 2c7fbee
Fix repro-querybug.egg
FTRobbin 7664d1a
Fix repro-querybug3.egg
FTRobbin 56f44d1
Fix repro-querybug4.egg
FTRobbin d6b652d
Fix repro-silly-panic.egg
FTRobbin b28153f
Fix resolution.egg
FTRobbin 84ab15c
Fix rw-analysis.egg
FTRobbin 299cda9
Fix set.egg
FTRobbin fa9c2af
Fix stresstest_large_expr.egg
FTRobbin cade37b
Fix tricky-type-checking.egg
FTRobbin 63d6d49
Fix typecheck.egg
FTRobbin d12ff1a
Fix typeinfer.egg
FTRobbin 3cb37a2
Fix unification-points-to.egg
FTRobbin f021915
Fix unstable-fn.egg
FTRobbin 76b6b75
Fix until.egg
FTRobbin cdfa007
Fix repro-duplicated-var.egg
FTRobbin 76c2b7f
Fix integration_test.rs
FTRobbin 539b62c
Enforcing the output type of constructors to be sort
FTRobbin 3d4e3b5
Add negative test constructor_non_sort.egg
FTRobbin 0a4cf65
Fix python_array_optimize.egg
FTRobbin 5e544b0
Fix stresstest_large_expr.egg
FTRobbin 5c65ba6
Fix integration_test.rs
FTRobbin 9437825
Fix python_array_optimize.egg
FTRobbin 3af6e07
Fix stresstest_large_expr.egg
FTRobbin 8c76699
Disable union merge for functions
FTRobbin 2cdfd40
Add set_sort_function.egg
FTRobbin 27d646d
Delete combinators_function.egg
FTRobbin 31d45b3
Fix intersection.egg
FTRobbin 76025d9
Fix unification-points-to.egg
FTRobbin bee3b4c
Fix cyk.egg
FTRobbin 2c5f58f
Add negate test union_non_sort.egg
FTRobbin 768d943
Minor
FTRobbin aa35478
Fix eggcc-extraction.egg
FTRobbin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -367,6 +367,19 @@ impl TypeInfo { | |
let (query, mapped_query) = Facts(body.clone()).to_query(self, symbol_gen); | ||
constraints.extend(query.get_constraints(self)?); | ||
|
||
//Disallowing Let/Set actions to look up non-constructor functions in rules | ||
for action in head.iter() { | ||
match action { | ||
GenericAction::Let(_, _, Expr::Call(_, symbol, _)) => { | ||
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. Don't you need to check if this is a function vs a constructor call here? |
||
return Err(TypeError::LookupInRuleDisallowed(*symbol, span.clone())); | ||
} | ||
GenericAction::Set(_, _, _, Expr::Call(_, symbol, _)) => { | ||
return Err(TypeError::LookupInRuleDisallowed(*symbol, span.clone())); | ||
} | ||
_ => (), | ||
} | ||
} | ||
|
||
let mut binding = query.get_vars(); | ||
let (actions, mapped_action) = head.to_core_actions(self, &mut binding, symbol_gen)?; | ||
|
||
|
@@ -516,6 +529,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>), | ||
} | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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