Merge ast and parser from forward #3

Legotier · 2022-05-19T10:05:14Z

No description provided.

This does not adapt the tests to the new features, it just makes sure there are no errors due to the renaming of classes or changed parameters in functions

…to brand-new

None was never an allowed value for nodes in a networkx.DiGraph, but older versions of networkx apparently didn't throw an error when it was added

…at I just spent hours figuring out

This reverts commit 077a5eb.

…ressions" This reverts commit 2afe578.

Philipp15b

Thanks a lot, excellent work! I just added some comments. Please make sure each file has an empty line at the end (otherwise Github shows the ugly red icon at the end). There might also be a bug in the change to cfg.py. The linter found a bunch of minor problems, it would be great if you could fix them as well. See https://github.com/Philipp15b/probably/runs/6505098898?check_suite_focus=true.

We should also note all breaking changes in this pull request so we can update other tools that depend on it. I see the following:

Changed names FloatLitExpr to RealLitExpr
Changed operator precedence so that the : operator now binds less strong than multiplication (what does that mean in practice, do we have an example?)
Changed the constructor of the Program class and the Program.from_parse function to require one more parameter
Probably now depends on at least Python 3.9
Rename Assoc.Left to LEFT and Right to RIGHT
More?

probably/pgcl/ast.py

probably/pgcl/syntax.py

probably/pgcl/cfg.py

probably/pgcl/parser.py

Philipp15b · 2022-05-19T12:25:12Z

probably/pgcl/parser.py

+                return PlotInstr(VarExpr(_parse_var(_child_tree(t, 0))),
+                                 prob=lit)
+            elif t.children[1].data == 'true' or t.children[1].data == 'false':
+                raise SyntaxError("Plot instruction does not support boolean operators")


Again, belongs in the type checker.

Is this actually resolved?

I think there is actually a bug in the parsing of plot instructions. If we write !Plot[x, true] in pGCL, the true is interpreted as a VarExpr with value true instead of as an (illegal) boolean literal.

Which also raises the question, shouldn't it be forbidden to have variables etc. that are called "true" or "false"?

You're right! I guess the nicest way to solve this would be to parse full expressions in !Plot and then have a check somewhere that it's actually a variable. That way, we can give proper error messages.

I had hoped that the parser would reject any keywords as variables, e.g. true := whatever should give an error. If that's not the case, another bug! I'd solve that by manually rejecting all keywords such as true and false in the _parse_var function:

probably/probably/pgcl/parser.py

Lines 183 to 186 in 6158b37

def _parse_var(t: Tree) -> Var:

assert t.data == 'var'

assert t.children[0].type == 'CNAME' # type: ignore

return str(_child_str(t, 0))

I had hoped that the parser would reject any keywords as variables, e.g. true := whatever should give an error. If that's not the case, another bug!

Fixed by 0a598c2, at least for true and false (are there even more?).

I guess the nicest way to solve this would be to parse full expressions in !Plot and then have a check somewhere that it's actually a variable.

I feel like this is just treating the symptoms (i.e., retrospectively fixing that the tree generated by lark contains a var node where a literal should be) instead of fighting the actual cause (i.e., making sure that there isn't even a var node in the first place). Wouldn't it be more elegant to somehow change the lark grammar such that true and false are never recognized as variable names? If we do it like you propose, this check needs to occur after the parsing is finished, or we need a global list in the parser that contains all variables in the current program (similar to the parameter dict). Both of these possibilities seem wrong to me.

For example, commit 12167a5 does this. This fixes both of these problems, it just leads to some very cryptic error messages when a variable has an illegal name.

If we do it like you propose, this check needs to occur after the parsing is finished, or we need a global list in the parser that contains all variables in the current program (similar to the parameter dict)

I don't understand that. Just check whether a variable name is a keyword and throw an error if that's the case. I think that's the easiest way to do this. As an additional safety measure, the constructor of VarExpr could assert that the name is not a keyword.

it just leads to some very cryptic error messages when a variable has an illegal name

This is exactly why we should be liberal in what we parse and then provide useful error messages afterwards.

Fixed, more or less. It's still not in the typechecker, but wrongly typed code can't be parsed anyways in this case.

probably/pgcl/check.py

probably/pgcl/syntax.py

probably/pgcl/check.py

probably/pgcl/parser.py

probably/pgcl/syntax.py

This reverts commit 12167a5.

Legotier · 2022-06-08T12:55:16Z

probably/pgcl/parser.py

@@ -441,7 +441,8 @@ def _parse_query(t: Tree):
                assert isinstance(lit, RealLitExpr)
                return PlotInstr(VarExpr(_parse_var(_child_tree(t, 0))),
                                 prob=lit)
-            elif t.children[1].data in ('true', 'false'):
+            elif t.children[1].data in ('true', 'false') or \
+                    (t.children[1].data == 'var' and t.children[1].children[0] in ('true', 'false')):
                raise SyntaxError("Plot instruction does not support boolean operators")


This is a workaround for the problem mentioned here, so we get a nicer error message. This check should still be moved to the typechecker, but I don't see how this can be done without some extensive rewrites, as the parsing of a PlotInstr already depends on the type of the literal. Also, the parser throws an error that there is an illegal variable name anyways, this change here just changes the error message.

Philipp15b · 2022-06-09T16:43:07Z

Makefile

+
+format:
+	poetry run yapf -r -i probably/ tests/
+	poetry run isort probably/ tests/


isort is not a dev-dependency, does this actually work?

It runs on my machine... Maybe some other package depends on isort? In any case, if this should stay here then we should probably add it to the dependencies

Legotier and others added 24 commits May 6, 2022 08:53

Update poetry configuration

53844ed

Add testreport to gitignore

e6a9c16

Merge new stuff from forward into ast.py

0e4ffaa

Minimal changes to make the tests pass

7fbc8bd

This does not adapt the tests to the new features, it just makes sure there are no errors due to the renaming of classes or changed parameters in functions

Rename float types to real types

4fc36c1

Update docs to include new classes

c296998

Use autodata instead of autoclass for union types

9d167e3

changed workflow to python version 3.9

7adbe0f

Enable type aliases for union types in the docs

7021d76

Merge branch 'brand-new' of https://github.com/Philipp15b/probably in…

816dc31

…to brand-new

Remove forward type references in ast.py

697e1d1

resolved dep issues

b2ede13

Disallow edges to None in ControlFlowGraph.to_networkx()

1d5af7a

None was never an allowed value for nodes in a networkx.DiGraph, but older versions of networkx apparently didn't throw an error when it was added

Merge parser from forward

6eb0df9

A helpful comment so people don't have to spend hours figuring out wh…

b8f7cf3

…at I just spent hours figuring out

Update test_uniform_checks

15f59e7

Fix precedence of likelyhood operator

9012370

Add test for basic operator precedence

077a5eb

Revert "Add test for basic operator precedence"

f8b3e08

This reverts commit 077a5eb.

Add test for basic operator precedence

9cc3a75

Test for correct parsing of distributions

8833b7f

Test for using minus with likely

3835e4d

Only allow natural numbers and parameters in distribution expressions

2afe578

Revert "Only allow natural numbers and parameters in distribution exp…

7912125

…ressions" This reverts commit 2afe578.

Philipp15b reviewed May 19, 2022

View reviewed changes

Legotier added 4 commits May 20, 2022 10:02

Minor fixes

ce569fb

Revert to older version (2.5) of networkx

c179a25

Remove test for parser check that is no longer done

5de84e6

Clear parameter dict for every new program that is parsed

a3a3246

This comment was marked as duplicate.

Sign in to view

LKlinke reviewed Jun 2, 2022

View reviewed changes

probably/pgcl/check.py Show resolved Hide resolved

Replace not_a_variable and add a warning to the typechecker

0da4e94

Legotier commented Jun 2, 2022

View reviewed changes

probably/pgcl/check.py Outdated Show resolved Hide resolved

Make it more explicit that iid type checking is unsafe

6158b37

Philipp15b reviewed Jun 3, 2022

View reviewed changes

probably/pgcl/check.py Outdated Show resolved Hide resolved

Philipp15b reviewed Jun 3, 2022

View reviewed changes

probably/pgcl/syntax.py Outdated Show resolved Hide resolved

Philipp15b reviewed Jun 3, 2022

View reviewed changes

probably/pgcl/syntax.py Outdated Show resolved Hide resolved

Philipp15b reviewed Jun 3, 2022

View reviewed changes

probably/pgcl/check.py Outdated Show resolved Hide resolved

Philipp15b reviewed Jun 3, 2022

View reviewed changes

probably/pgcl/parser.py Outdated Show resolved Hide resolved

Philipp15b reviewed Jun 3, 2022

View reviewed changes

probably/pgcl/syntax.py Outdated Show resolved Hide resolved

Legotier added 6 commits June 3, 2022 14:25

Logger warning for typechecking of iid exprs

7e8c180

Update parameter in syntax module

6247646

Fix warnings

bb071e1

Disallow constants in programs in the syntax module

579fdf6

Make some variable names (true, false) illegal

0a598c2

Eliminate true and false from pgcl grammar

12167a5

Philipp15b reviewed Jun 3, 2022

View reviewed changes

probably/pgcl/syntax.py Outdated Show resolved Hide resolved

Legotier added 4 commits June 8, 2022 13:53

Revert "Eliminate true and false from pgcl grammar"

e606806

This reverts commit 12167a5.

Fix error message in check_is_linear_expr

4d4c322

Add assertion for illegal keyword to constructor of VarExpr

21ae331

Better error message for usage of true or false in plot instructions

f1d923a

Legotier commented Jun 8, 2022

View reviewed changes

Legotier added 5 commits June 9, 2022 13:38

Allow variables in distribution parameters

79991b9

Remove program config

1777e88

Add typecheck for TickInstr and TickExpr

3af0bf5

Auto format

2dd1aa3

Add format target to makefile

a2adf9f

Philipp15b reviewed Jun 9, 2022

View reviewed changes

Philipp15b approved these changes Jun 10, 2022

View reviewed changes

Philipp15b merged commit ce51260 into master Jun 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge ast and parser from forward #3

Merge ast and parser from forward #3

Legotier commented May 19, 2022

Philipp15b left a comment •

edited by Legotier

Loading

Philipp15b May 19, 2022

Philipp15b Jun 3, 2022

Legotier Jun 3, 2022

Legotier Jun 3, 2022 •

edited

Loading

Philipp15b Jun 3, 2022

Legotier Jun 3, 2022 •

edited

Loading

Philipp15b Jun 3, 2022

Legotier Jun 9, 2022

This comment was marked as duplicate.

Legotier Jun 8, 2022 •

edited

Loading

Philipp15b Jun 9, 2022

Legotier Jun 9, 2022

	def _parse_var(t: Tree) -> Var:
	assert t.data == 'var'
	assert t.children[0].type == 'CNAME' # type: ignore
	return str(_child_str(t, 0))

Merge ast and parser from forward #3

Merge ast and parser from forward #3

Conversation

Legotier commented May 19, 2022

Philipp15b left a comment • edited by Legotier Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Legotier Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Legotier Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Legotier Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Philipp15b left a comment •

edited by Legotier

Loading

Legotier Jun 3, 2022 •

edited

Loading

Legotier Jun 3, 2022 •

edited

Loading

Legotier Jun 8, 2022 •

edited

Loading