Rule Set Classification#610
Conversation
… efficient) refactoring
monsterkrampe
left a comment
monsterkrampe
left a comment
There was a problem hiding this comment.
In general, the structure looks good. Don't be discouraged by the number of comments :)
The list of things to address is certainly not exhaustive and I did not verify the checks for correctness. I only had a closer look at some of the details.
Anyway, there should be enough things to address already.
We can (and maybe should) discuss some of this offline of course :)
| pub struct AnyDataValue(AnyDataValueEnum); | ||
|
|
||
| impl AnyDataValue { | ||
| // TODO: DELETE FUNCTION ONCE MFA CHECK CAN BE IMPLEMENTED DIFFERENTLY |
There was a problem hiding this comment.
Is this still relevant? If so, the comment should give more details about what the requirements are for implementing MFA "differently" and what "differently" means in the first place.
Also, can this move out of this crate and be monkey patched in nemo-static-checks (if still required)?
| // TODO: DELETE FUNCTION ONCE MFA CHECK CAN BE IMPLEMENTED DIFFERENTLY | ||
| pub(crate) fn plain_string(&self) -> &String { | ||
| &self.0 | ||
| } |
There was a problem hiding this comment.
Why is lexical_value or to_plain_string_unchecked not enough? Or in other words: why do you need a reference to the string and not its value?
| "nemo-cli", | ||
| "nemo-physical", | ||
| "nemo-python", | ||
| #"nemo-python", |
There was a problem hiding this comment.
I think this should be either kept or removed entirely instead of just commented out. (Also below.)
There was a problem hiding this comment.
MacOS specific files should not be committed. Maybe this was added before the .gitignore had been adjusted?
There was a problem hiding this comment.
This file is called .rlsd but links to .rls file. This is wrong in a couple of places, sometimes also the other way around. Please double check and fix this.
Also, am I right to assume that the d stands for "disjunctive"?
| } | ||
| } | ||
|
|
||
| pub fn contains_func(&self) -> bool { |
There was a problem hiding this comment.
This should only exists in the static checks crate.
| /// | ||
| /// Basic building block for expressions like atoms or facts. | ||
| #[derive(Debug, Clone, PartialEq, Hash, PartialOrd)] | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd)] |
There was a problem hiding this comment.
I do not think that this should implement Eq. For example, I think that a term could also have the floating point value NaN. But NaN != NaN so reflexivity guaranteed by Eq would be violated.
| #[derive(Debug, Default, Clone, Copy)] | ||
| pub struct TransformationCriticalInstance {} | ||
|
|
||
| fn critical_instance(rules: &[&Rule]) -> HashSet<Fact> { |
There was a problem hiding this comment.
Are these rules allowed to feature constants? If so, the critical instance needs to involve these constants too, I think. I need to double check the right approach to handle this. Talk to me if constants in rules should be supported.
| #[derive(Debug, Default, Clone, Copy)] | ||
| pub struct TransformationMSA {} | ||
|
|
||
| fn f_preds_for_ex_vars(ex_vars_of_rule: &HashSet<&Variable>, rule_name: usize) -> Vec<Tag> { |
There was a problem hiding this comment.
I'm confused here. Doesn't MSA simply replace each existential variable by a fresh constant? Why do you need new predicates? What am I missing? (Maybe I'm also misremembering how MSA works.)
| }) | ||
| .unwrap(); | ||
| let front_vars = rule.frontier_variables(); | ||
| let mut const_count = 0; |
There was a problem hiding this comment.
I think the fresh constants need to be globally unique. This is not ensured here is it? What I mean is, they are only unique within this one rules and not across the rule set, right?
3 participants
This PR will add membership checks for various syntactic (and some semantic) classes of rule sets. The implementation is done by @xR0xEr.
(This will still take some time and I am mainly creating this Draft PR now to have a place for review and discussion.)