parse-rules fails on any unknown variable

[latacora-gabriel]

Hi, Thanks for the handy library! I was looking to use datalog.parser.impl/parse-rules for linting rules. I noticed that the following throws an error Reference to the unknown variables: (?region ?account ?call) even though it's a valid rule and the unbound variables serve a purpose:

[(aws-vpc-attr ?id ?attr ?val)
       ["aws" ?account ?region "ec2" "describe-vpc-attribute" ?call :vpc-id ?id]
       ["aws" ?account ?region "ec2" "describe-vpc-attribute" ?call ?attr ?val]]

Is this the desired behavior? Also curious why this in impl as it'd probably be useful for others.

Cheers, Gabriel

[whilo]

My understanding is that in Datomic's Datalog you are always supposed to expose all free variables as arguments in the rule head https://docs.datomic.com/on-prem/query/query.html#rules and then you can still decide whether you bind them in your context. But I agree that there are cases where you would like to want to hide this detail. Can you invoke this rule in Datomic actually? We do not have to stick to Datomic, but so far we followed DataScript and Datomic closely.

[latacora-gabriel]

I'm able to run the above rule in datascript fine. I'm not sure about datomic's behavior. https://docs.datomic.com/cloud/query/query-data-reference.html#rule-required-bindings shows an example where the variable ?track isn't exposed as a rule arg much like the unknown variables in my example

[whilo]

Then we probably lack a backport from a fix to DataScript's parser. If it is not too much of a hassle for you, it would be super nice if you could give it a shot. If not, no problem, maybe I can take a look at it over the weekend.

[latacora-gabriel]

Unfortunately it'll probably be at least a month before I may use parse-rules for this case at work. I'm not in urgent need of this so I'm ok with just having had this conversation :) I'm not sure if I should use this fn since it's an impl ns but I'm guessing it's ok

On Tue, Dec 7, 2021 at 1:16 PM Christian Weilbach @.***> wrote:

Then we probably lack a backport from a fix to DataScript's parser. If it is not too much of a hassle for you, it would be super nice if you could give it a shot. If not, no problem, maybe I can take a look at it over the weekend.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/lambdaforge/datalog-parser/issues/20#issuecomment-988161214, or unsubscribe https://github.com/notifications/unsubscribe-auth/AT3CAAVUYN2PRUMG6NJYG53UPZFRPANCNFSM5JFGVO6A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[zoren]

It looks like validate-vars should be removed. That's what happened in datascript. https://github.com/tonsky/datascript/commit/2f1352d7f54e89d7ade362f2295b62e1c8d40fa3#diff-a8f9d8f87089a54669aa37ba7c57edc21675f87758a6ef09cfbd1a5d71b48b1a

[whilo]

@zoren Thanks for the pointer! @jsmassa I think this could be related to your recent fixes for the constants in the rule syntax, what do you think?

[zoren]

It seems that datahike doesn't call parse-rules, is that on purpose?

[jsmassa]

@whilo I didn't touch the parser for this fix, in the end it was just a fix in Datahike.

@zoren I am wondering about that, too, now. Maybe @kordano knows more about this?

[jsmassa]

Btw, does the parse-rules? function still fail? I saw, there was some movement in this repo a couple of days ago, so I was wondering if it concerned this issue or even solved the problem.

[zoren]

Btw, does the parse-rules? function still fail?

It does, the problem is here https://github.com/lambdaforge/datalog-parser/blob/master/src/datalog/parser/impl.cljc#L466 rules should be allowed to use variables not bound as parameters.

[cldwalker]

With the latest 0.1.10, I ran into this again with the following query which is valid in datascript:

[(block-property ?b ?property-name ?property-value)
   [?b :block/properties ?p]
   [(get ?p ?property-name) ?property-value]]

The error was Reference to the unknown variables: (?p)

@whilo @zoren Would you all be open to a pull request that removes validate-vars like datascript did?

[zoren]

@whilo @zoren Would you all be open to a pull request that removes validate-vars like datascript did?

I’m not an owner but I do think the current behavior is wrong and should be changed.

[kordano]

Yeah, sure you can create a PR.