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

refactor: Update function checker to use new diagnostics #590

Merged
merged 7 commits into from
Oct 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions examples/demo.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -486,13 +486,18 @@
"name": "stderr",
"output_type": "stream",
"text": [
"Guppy compilation failed. Error in file <In [14]>:6\n",
"Error: Illegal assignment (at <In [14]>:6:8)\n",
" | \n",
"4 | def outer(x: int) -> int:\n",
"5 | def nested() -> None:\n",
"6 | x += 1 # Mutation of captured variable x is not allowed\n",
" | ^^^^^^ Variable `x` may not be assigned to since `x` is captured\n",
" | from an outer scope\n",
" | \n",
"4 | def outer(x: int) -> int:\n",
" | ------ `x` defined here\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be cool to detect overlapping spans and only print the outer once like

def outer(x: int) -> int
                ----- `x` defined here
    def nested() -> None
        x += 1
        ^^^^^^ Variable `x`...

but that seems tricky...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, good idea, Rust also does this! Added to #608

"\n",
"4: def outer(x: int) -> int:\n",
"5: def nested() -> None:\n",
"6: x += 1 # Mutation of captured variable x is not allowed\n",
" ^^^^^^\n",
"GuppyError: Variable `x` defined in an outer scope (at 4:10) may not be assigned to\n"
"Guppy compilation failed due to 1 previous error\n"
]
}
],
Expand Down
9 changes: 9 additions & 0 deletions guppylang/checker/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from typing import (
TYPE_CHECKING,
Any,
ClassVar,
Generic,
NamedTuple,
TypeAlias,
Expand All @@ -20,6 +21,7 @@
from guppylang.definition.common import DefId, Definition
from guppylang.definition.ty import TypeDef
from guppylang.definition.value import CallableDef
from guppylang.diagnostic import Error
from guppylang.tys.builtin import (
array_type_def,
bool_type_def,
Expand Down Expand Up @@ -49,6 +51,13 @@
from guppylang.definition.struct import StructField


@dataclass(frozen=True)
class UnsupportedError(Error):
title: ClassVar[str] = "Unsupported"
span_label: ClassVar[str] = "{things} are not supported"
things: str


#: A "place" is a description for a storage location of a local value that users
#: can refer to in their program.
#:
Expand Down
68 changes: 51 additions & 17 deletions guppylang/checker/func_checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,16 @@
"""

import ast
from typing import TYPE_CHECKING
from dataclasses import dataclass
from typing import TYPE_CHECKING, ClassVar

from guppylang.ast_util import return_nodes_in_ast, with_loc
from guppylang.cfg.bb import BB
from guppylang.cfg.builder import CFGBuilder
from guppylang.checker.cfg_checker import CheckedCFG, check_cfg
from guppylang.checker.core import Context, Globals, Place, Variable
from guppylang.checker.core import Context, Globals, Place, UnsupportedError, Variable
from guppylang.definition.common import DefId
from guppylang.diagnostic import Error, Help, Note
from guppylang.error import GuppyError
from guppylang.nodes import CheckedNestedFunctionDef, NestedFunctionDef
from guppylang.tys.parsing import parse_function_io_types
Expand All @@ -23,6 +25,41 @@
from guppylang.tys.param import Parameter


@dataclass(frozen=True)
class IllegalAssignError(Error):
title: ClassVar[str] = "Illegal assignment"
span_label: ClassVar[str] = (
"Variable `{var}` may not be assigned to since `{var}` is captured from an "
"outer scope"
)
var: str

@dataclass(frozen=True)
class DefHint(Note):
span_label: ClassVar[str] = "`{var}` defined here"
var: str


@dataclass(frozen=True)
class MissingArgAnnotationError(Error):
title: ClassVar[str] = "Missing type annotation"
span_label: ClassVar[str] = "Argument requires a type annotation"


@dataclass(frozen=True)
class MissingReturnAnnotationError(Error):
title: ClassVar[str] = "Missing type annotation"
span_label: ClassVar[str] = "Return type must be annotated"

@dataclass(frozen=True)
class ReturnNone(Help):
message: ClassVar[str] = (
"Looks like `{func}` doesn't return anything. Consider annotating it with "
"`-> None`."
)
func: str


def check_global_func_def(
func_def: ast.FunctionDef, ty: FunctionType, globals: Globals
) -> CheckedCFG[Place]:
Expand Down Expand Up @@ -67,12 +104,9 @@ def check_nested_func_def(
for v, _ in captured.values():
x = v.name
if x in bb.vars.assigned:
raise GuppyError(
f"Variable `{x}` defined in an outer scope (at {{0}}) may not "
f"be assigned to",
bb.vars.assigned[x],
[v.defined_at],
)
err = IllegalAssignError(bb.vars.assigned[x], x)
err.add_sub_diagnostic(IllegalAssignError.DefHint(v.defined_at, x))
raise GuppyError(err)

# Construct inputs for checking the body CFG
inputs = [v for v, _ in captured.values()] + [
Expand Down Expand Up @@ -123,24 +157,24 @@ def check_signature(func_def: ast.FunctionDef, globals: Globals) -> FunctionType
Guppy type."""
if len(func_def.args.posonlyargs) != 0:
raise GuppyError(
"Positional-only parameters not supported", func_def.args.posonlyargs[0]
UnsupportedError(func_def.args.posonlyargs[0], "Positional-only parameters")
)
if len(func_def.args.kwonlyargs) != 0:
raise GuppyError(
"Keyword-only parameters not supported", func_def.args.kwonlyargs[0]
UnsupportedError(func_def.args.kwonlyargs[0], "Keyword-only parameters")
)
if func_def.args.vararg is not None:
raise GuppyError("*args not supported", func_def.args.vararg)
raise GuppyError(UnsupportedError(func_def.args.vararg, "Variadic args"))
if func_def.args.kwarg is not None:
raise GuppyError("**kwargs not supported", func_def.args.kwarg)
raise GuppyError(UnsupportedError(func_def.args.kwarg, "Keyword args"))
if func_def.returns is None:
err = MissingReturnAnnotationError(func_def)
# TODO: Error location is incorrect
if all(r.value is None for r in return_nodes_in_ast(func_def)):
raise GuppyError(
"Return type must be annotated. Try adding a `-> None` annotation.",
func_def,
err.add_sub_diagnostic(
MissingReturnAnnotationError.ReturnNone(None, func_def.name)
)
raise GuppyError("Return type must be annotated", func_def)
raise GuppyError(err)

# TODO: Prepopulate mapping when using Python 3.12 style generic functions
param_var_mapping: dict[str, Parameter] = {}
Expand All @@ -149,7 +183,7 @@ def check_signature(func_def: ast.FunctionDef, globals: Globals) -> FunctionType
for inp in func_def.args.args:
ty_ast = inp.annotation
if ty_ast is None:
raise GuppyError("Argument type must be annotated", inp)
raise GuppyError(MissingArgAnnotationError(inp))
input_nodes.append(ty_ast)
input_names.append(inp.arg)
inputs, output = parse_function_io_types(
Expand Down
8 changes: 7 additions & 1 deletion guppylang/definition/function.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,13 @@ def parse_py_func(f: PyFunc, sources: SourceMap) -> tuple[ast.FunctionDef, str |
defn = find_ipython_def(func_ast.name)
if defn is not None:
file = f"<{defn.cell_name}>"
sources.add_file(file, source)
sources.add_file(file, defn.cell_source)
else:
# If we couldn't find the defining cell, just use the source code we
# got from inspect. Line numbers will be wrong, but that's the best we
# can do.
sources.add_file(file, source)
line_offset = 1
else:
file = inspect.getsourcefile(f)
if file is None:
Expand Down
15 changes: 13 additions & 2 deletions guppylang/diagnostic.py
Original file line number Diff line number Diff line change
Expand Up @@ -208,10 +208,15 @@ def render_diagnostic(self, diag: Diagnostic) -> None:
else:
span = to_span(diag.span)
level = self.level_str(diag.level)
all_spans = [span] + [
to_span(child.span) for child in diag.children if child.span
]
max_lineno = max(s.end.line for s in all_spans)
self.buffer.append(f"{level}: {diag.rendered_title} (at {span.start})")
self.render_snippet(
span,
diag.rendered_span_label,
max_lineno,
is_primary=True,
prefix_lines=self.PREFIX_CONTEXT_LINES,
)
Expand All @@ -221,6 +226,7 @@ def render_diagnostic(self, diag: Diagnostic) -> None:
self.render_snippet(
to_span(sub_diag.span),
sub_diag.rendered_span_label,
max_lineno,
is_primary=False,
)
if diag.message:
Expand All @@ -238,7 +244,12 @@ def render_diagnostic(self, diag: Diagnostic) -> None:
)

def render_snippet(
self, span: Span, label: str | None, is_primary: bool, prefix_lines: int = 0
self,
span: Span,
label: str | None,
max_lineno: int,
is_primary: bool,
prefix_lines: int = 0,
) -> None:
"""Renders the source associated with a span together with an optional label.

Expand Down Expand Up @@ -272,7 +283,7 @@ def render_snippet(
additional context.
"""
# Check how much space we need to reserve for the leading line numbers
ll_length = len(str(span.end.line))
ll_length = len(str(max_lineno))
highlight_char = "^" if is_primary else "-"

def render_line(line: str, line_number: int | None = None) -> None:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
Error: Can't compare apples with oranges (at <unknown>:99:6)
|
97 | apple == orange
98 | apple == orange
99 | apple == orange
Copy link
Collaborator

Choose a reason for hiding this comment

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

😁

| ^^ Comparison attempted here
|
100 | This apple is on another line
| ----- This is an apple
13 changes: 13 additions & 0 deletions tests/diagnostics/test_diagnostics_rendering.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,19 @@ def test_justify_line_number(snapshot, request):
run_test(source, diagnostic, snapshot, request)


def test_two_spans_different_lineno_lens(snapshot, request):
@dataclass(frozen=True)
class MySubDiagnostic(Note):
span_label: ClassVar[str] = "This is an apple"

source = 99 * "apple == orange\n" + "This apple is on another line"
span1 = Span(Loc(file, 99, 6), Loc(file, 99, 8))
span2 = Span(Loc(file, 100, 5), Loc(file, 100, 10))
diagnostic = MyError(span1)
diagnostic.add_sub_diagnostic(MySubDiagnostic(span2))
run_test(source, diagnostic, snapshot, request)


def test_indented(snapshot, request):
source = " " * 50 + "super_apple := apple ** 2\n"
source += " " * 50 + " lemon := orange - apple\n"
Expand Down
12 changes: 7 additions & 5 deletions tests/error/misc_errors/arg_not_annotated.err
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
Guppy compilation failed. Error in file $FILE:5
Error: Missing type annotation (at $FILE:5:17)
|
3 |
4 | @compile_guppy
5 | def foo(x: bool, y) -> int:
| ^ Argument requires a type annotation

3: @compile_guppy
4: def foo(x: bool, y) -> int:
^
GuppyError: Argument type must be annotated
Guppy compilation failed due to 1 previous error
14 changes: 9 additions & 5 deletions tests/error/misc_errors/return_not_annotated.err
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
Guppy compilation failed. Error in file $FILE:5
Error: Missing type annotation (at $FILE:5:0)
|
3 |
4 | @compile_guppy
5 | def foo(x: bool):
| ^^^^^^^^^^^^^^^^^
6 | return x
| ^^^^^^^^^^^^ Return type must be annotated

Copy link
Collaborator

Choose a reason for hiding this comment

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

it'd be nice if this could highlight the foo signature instead of the whole function...
Would it be better to have the message be "return type must be specified in method signature"?

Copy link
Collaborator Author

@mark-koch mark-koch Oct 31, 2024

Choose a reason for hiding this comment

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

Unfortunately, the Python AST doesn't give us a span for this. Taking the first line is also not valid since you could have weird stuff like

def foo(
  # No arguments, so no argument spans
)     :   # Closing paren on separate line + extra spaces before the colon

3: @compile_guppy
4: def foo(x: bool):
^^^^^^^^^^^^^^^^^
GuppyError: Return type must be annotated
Guppy compilation failed due to 1 previous error
17 changes: 12 additions & 5 deletions tests/error/misc_errors/return_not_annotated_none1.err
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
Guppy compilation failed. Error in file $FILE:5
Error: Missing type annotation (at $FILE:5:0)
|
3 |
4 | @compile_guppy
5 | def foo():
| ^^^^^^^^^^
6 | return
| ^^^^^^^^^^ Return type must be annotated

3: @compile_guppy
4: def foo():
^^^^^^^^^^
GuppyError: Return type must be annotated. Try adding a `-> None` annotation.
Help: Looks like `foo` doesn't return anything. Consider annotating it with `->
None`.

Guppy compilation failed due to 1 previous error
18 changes: 13 additions & 5 deletions tests/error/misc_errors/return_not_annotated_none2.err
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
Guppy compilation failed. Error in file $FILE:5
Error: Missing type annotation (at $FILE:5:0)
|
3 |
4 | @compile_guppy
5 | def foo():
| ^^^^^^^^^^
| ...
7 | return x
| ^^^^^^^^^^^^^^^^ Return type must be annotated

3: @compile_guppy
4: def foo():
^^^^^^^^^^
GuppyError: Return type must be annotated. Try adding a `-> None` annotation.
Help: Looks like `foo` doesn't return anything. Consider annotating it with `->
None`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔥


Guppy compilation failed due to 1 previous error
17 changes: 11 additions & 6 deletions tests/error/nested_errors/reassign_capture_1.err
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
Guppy compilation failed. Error in file $FILE:9
Error: Illegal assignment (at $FILE:9:8)
|
7 |
8 | def bar() -> None:
9 | y += 2
| ^^^^^^ Variable `y` may not be assigned to since `y` is captured
| from an outer scope
|
6 | y = x + 1
| - `y` defined here

7:
8: def bar() -> None:
9: y += 2
^^^^^^
GuppyError: Variable `y` defined in an outer scope (at 6:4) may not be assigned to
Guppy compilation failed due to 1 previous error
17 changes: 11 additions & 6 deletions tests/error/nested_errors/reassign_capture_2.err
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
Guppy compilation failed. Error in file $FILE:11
Error: Illegal assignment (at $FILE:11:8)
|
9 | if 3 > 2:
10 | z = y
11 | y = 2
| ^^^^^ Variable `y` may not be assigned to since `y` is captured
| from an outer scope
|
6 | y = x + 1
| - `y` defined here

9: if 3 > 2:
10: z = y
11: y = 2
^^^^^
GuppyError: Variable `y` defined in an outer scope (at 6:4) may not be assigned to
Guppy compilation failed due to 1 previous error
17 changes: 11 additions & 6 deletions tests/error/nested_errors/reassign_capture_3.err
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
Guppy compilation failed. Error in file $FILE:11
Error: Illegal assignment (at $FILE:11:12)
|
9 |
10 | def baz() -> None:
11 | y += 2
| ^^^^^^ Variable `y` may not be assigned to since `y` is captured
| from an outer scope
|
6 | y = x + 1
| - `y` defined here

Copy link
Collaborator

Choose a reason for hiding this comment

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

is it worth sorting the code spans based on the lines they appear in the file? it looks a little bit like this is one snippet and y is defined after baz

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's a good idea. However, we'll need to be careful because the grammar might break in some cases when reordering:

Error: Linearity violation (at $FILE:21:8)
   | 
19 | def test(q: qubit @owned) -> None:
20 |     use(q)
21 |     foo(q)
   |         ^ Variable `q` with linear type `qubit` cannot be borrowed
   |           ...
   | 
20 |     use(q)
   |         - since it was already consumed here

We'd need two version of the span labels depending on which order they are outputted. I created #608 to collect these improvement ideas

9:
10: def baz() -> None:
11: y += 2
^^^^^^
GuppyError: Variable `y` defined in an outer scope (at 6:4) may not be assigned to
Guppy compilation failed due to 1 previous error
Loading