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

Add renamer phase and a simala backend for natural4 #609

Merged
merged 46 commits into from
Aug 26, 2024
Merged

Conversation

fendor
Copy link
Contributor

@fendor fendor commented Aug 13, 2024

This PR introduces two new things:

  • A Renamer/Name resolver phase for Rules
  • A Transpiler which allows transpiling a Rule to a Simala Decl.

The renamer implements lexical scope checking for Hornlike and TypeDecl rules.
It resolves all names, replacing occurrences of each name with its renamed equivalent.
This is supposed to lay the groundwork for a principled transpilation pipeline, avoiding redoing work for each transpiler, such as resolving names, making sure variables are in scope, etc...
In the future, we want to implement a type checker on top of a RnRule. However, that's currently only a dream and there are no attempts of doing any kind of typechecking at the moment.

The simala transpiler solely operates on the renamed rules. This simplifies the implementation a lot, demonstrating how other transpilers may also benefit from the rename phase.
The simala backend can currently only handle Hornlike rules and ignores any kind of TypeDecl rules, as simala is dynamically typed and typechecking should occur in a dedicated rule phase, not ad-hoc in the transpiler.

Both components are somewhat experimental and are expected to have bugs and missing features.

Todos:

  • Logging (potentially using colog style logging)
  • A proper error type (Don't throw Strings)
  • Integration with the main-cli

fendor and others added 27 commits August 13, 2024 13:53
Start documentation for the overall pipeline.
This was an attempt to use Tries for lexical scope checking. However,
after talking a little bit more about this, we decided to change this a
simpler implementation.
Scan the clauses of rules for variables they may have introduced.
Rename in a separate step.
@fendor fendor marked this pull request as draft August 13, 2024 12:31
lib/haskell/hie.yaml Outdated Show resolved Hide resolved
@fendor fendor marked this pull request as ready for review August 19, 2024 15:03
@fendor fendor changed the title Draft: Add renamer phase and a simala backend for natural4 Add renamer phase and a simala backend for natural4 Aug 20, 2024
@ym-han
Copy link
Contributor

ym-han commented Aug 21, 2024

Some quick feedback:

Renamer.hs

High level things

  • Would be helpful to have a comment or two indicating how to use the Renamer (might be enough to just indicate what the main entrypoints are in the export list and link to the Simala transpiler as an example?)
  • Might be helpful to have a quick comment that
    (i) summarizes what the assumptions and guarantees of the Renamer are
    (ii) summarizes why the renaming is sound, if there is an easy way to explain this at a high level --- there may not
  • A quick, high level summary of the scoping rules / semantics that have been implemented may also be helpful, though maybe it's obvious enough

Specific things

  • RnRelationalPredicate: Might be good to provide examples of the various variants, especially RnRelationalTerm. This isn't high-priority / doesn't have to be done in this PR. (And yes, any unclarity here really has to do with the unclarity in the original RelationalPredicate data structure, and not with what you have done.)
  • * @x@: a variable, might be bound ad-hoc -- what does 'bound ad-hoc' mean in this context?

In case the above came off as being negative: Again, I think that this PR has been a truly heroic amount of work.

Copy link
Contributor

@inariksit inariksit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Most of my comments are just minor remarks ("a comment rather than a question"). I thought the code was well documented, but of course, true test comes at the point when one starts trying to work with the code, not just passively look at it.

Comment on lines +779 to +781
-- TODO: error handling, would we accept an enum such as `a IS ONE OF 1, 2, 3`?
-- Only if we treat them as text, which might be confusing, as user might infer
-- this to be some kind of type checked number type.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this could be used for things like number of children (e.g. < 100) or birth year (e.g. > 1900). Even in such cases, the validation would be given as a range rather than an enum. Probably the most practical question is, what kind of error messages we'd like to give if a user claims to be born in 1700: is it a type error, or just a runtime result "you're too old for insurance"?

(There's also the TYPICALLY keyword, but I don't know of its status in any of the various transpilers or previous case studies.)

Comment on lines +935 to +936
renameLocalRules :: Tracer Log -> [Rule] -> Renamer [RnRule]
renameLocalRules = renameRules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to be replaced with something else later?

I'm asking, because earlier you wrote that GIVETHs are global, GIVENs are local and DECIDE head term in "IS" clauses is global. Is that the case in WHERE clauses as well? (Or am I mistaken about what renameRules does?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in lexical scoping, we have a hierarchy of scopes. GIVETH and DECIDE are not "global", but their definitions may be used one level up the hierarchy. This generalises nicely, as rules in WHERE can be used by DECIDE and other rules in the WHERE clause, but not outside the rule that defines the WHERE clause.

For a practical example with nested clauses:

GIVEN x
DECIDE f x IS y
WHERE
  y IS g x
  GIVEN d DECIDE g d IS y WHERE y IS SUM(d, d)
  • Then the rules y IS g x and GIVEN d DECIDE g d IS y WHERE y IS SUM(d, d) is local to the rule and the function g may be used in both WHERE and DECIDE blocks.
  • y IS SUM(d,d) is local to the rule g, and can not be referenced by the top-level WHERE or DECIDE.
  • y IS SUM(d,d) name shadows the outer y.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for the explanation!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another tangential syntax thing, but it's not clear to me that we really should allow users the flexibility to write arbitrary declarations in a WHERE block (as opposed to just things of the form y IS g x. There's probably no easy way to answer this question right now, and it's definitely not high-priority, but I'd be curious to see what you all think.

Comment on lines +1126 to +1133
-- TODO: this could be an expression such as "2+2" (for whatever reason), so perhaps
-- we need to parse this further. Allegedly, we also want to support
-- expressions nested into one csv-cell, for example:
--
-- >>> MT "f x y"
--
-- where 'f' is a function.
-- We ignore this for now, though.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question of what to support has always been a bit vague. If I remember correctly, arbitrary expressions in a single cell became an explicit thing around last spring, with these 4 test cases for (Generic)MathLang.
Simultaneously, we have been saying for many years "we can totally make NaturaL4 stricter and require the user to not just type in whatever". Maybe this could be a natural spot to make some decisions. 😛 @mengwong , any comments/opinions?

Comment on lines +235 to +236
-- TODO: handle multiple GIVETH's.
-- Actually, handle GIVETHs at all.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this comment mean that you plan to do so eventually (but it wasn't a prio for the first version), or that you are unsure how GIVETHs should be handled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose both, I want to focus on something that works with documented limitations, so that we can iteratively improve this version.

However, it is also unclear what a GIVETH really is, and how it would work in a function context, for example.

Comment on lines +1 to +2
Failed transpilation:
Unsupported RelationalPredicate: RPis
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the excpected result?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! This is intended as documenting the current capabilties of the transpilation.
I struggled to identify all possible meanings of predicates. In this case,

"Loss or Damage" IS ANY ( "caused by rodents"
                                    , "caused by insects"
                                    , "caused by vermin"
                                    , "caused by birds"
                                    )

Is a weird RelationalPredicate in my opinion. ANY feels overloaded. It may mean OR, but IS ANY suddenly means ELEM, and there is actually a RPelem RelationalPredicate which we don't use here.

So, I didn't even try to support this example, but rather we should talk about, which composite operators we want to support, or whether we rather want to use some ELEM operator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I pretty much have forgotten that RPelem existed 😁

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah in general I think we shouldn't be afraid to try to make the syntax more regimented and understandable (not least because there is a non-trivial chance we'll actually have to try to help other people learn and use this!)

lib/haskell/natural4/src/LS/Renamer.hs Outdated Show resolved Hide resolved
@fendor
Copy link
Contributor Author

fendor commented Aug 22, 2024

I tried to address some of the feedback, moved some code around that makes it hopefully a bit more clear what we are trying to do and added a documentation block that will hopefully evolve to a specification of the scoping rules in naturalL4.

A lot of good questions were raised regarding supported syntax and the type system, which we should write down and discuss.

I also fixed some glaring issues with the Simala Transpiler. However, I expect there are many issues left in the code, which we will hopefully weed out over time.
The renamer seems to be working relatively well, but is likely not tested enough. It would be nice to come up with a test specification that is better than simply printing a renamed rule, as this is really difficult to check, verify and understand.

After adding a test case, I realised the name shadowing did not work as
anticipated. We fix this now, move some code around and extend the
transpiler to work correctly for many more naturalL4 constructs and
fragments.
Copy link
Contributor

@ym-han ym-han left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding more docs / explanations!

lib/haskell/natural4/src/LS/Renamer.hs Show resolved Hide resolved
lib/haskell/natural4/src/LS/Renamer.hs Show resolved Hide resolved
lib/haskell/natural4/src/LS/Renamer.hs Show resolved Hide resolved
lib/haskell/natural4/src/LS/Renamer.hs Show resolved Hide resolved
@fendor fendor merged commit 91da336 into main Aug 26, 2024
3 checks passed
@fendor fendor deleted the fendor/backend/simala branch August 26, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants