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

Possible Id collision in compileToQuery #20

Open
Tarmean opened this issue Mar 5, 2023 · 1 comment
Open

Possible Id collision in compileToQuery #20

Tarmean opened this issue Mar 5, 2023 · 1 comment
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@Tarmean
Copy link

Tarmean commented Mar 5, 2023

compileToQuery generates new match ids starting from 0. But when users specify a VariablePattern n, n might collide with these generated IDs.
Note that collisions are very unlikely as long as users specify patterns with the IsString instance which uses fromString = VariablePattern . hashString.

The simplest fix would be something like let root :~ atoms = evalState (aux pa) (maximum (0:vars p)+1).

Unfortunately a fix requires some design decisions. There are two 'obvious' fixes: Compact user-given variables, or start name-generating form the largest user-given variable+1. The VariablePattern names are later used to retrieve results, so if the user-given names were compacted the Query would have to carry a Map UserVar InternalVar. And the hashString variables are evenly distributed, so starting at max+1 might be very large (but probably not so large that it would lead to an overflow).

Reproduction:

somePattern :: Pattern []
somePattern = NonVariablePattern [VariablePattern 0,VariablePattern 1] 

>compileToQuery  somePattern
(Query [0,1] [Atom (CVar 0) [CVar 0,CVar 1]], 0)

Note that the compiled pattern matches ?A = (?A,?B), i.e. a cyclic graph.

@alt-romes
Copy link
Owner

Thanks for spotting this!

Perhaps we could avoid the problem altogether by not exporting NonVariablePattern (though that might make the interface/documentation of patterns a bit weird), and by having the string instance be modulo some number (say 2000), and then generated IDs start from that number (2000).

In any case, if you'd like to attempt that, or one of the fixes you mentioned (choose according to your best judgment), I'd be happy to review+merge an MR.

@alt-romes alt-romes added bug Something isn't working good first issue Good for newcomers labels Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants