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

feat: Update top-level definitions to use new diagnostics #604

Merged
merged 4 commits into from
Nov 11, 2024

Conversation

mark-koch
Copy link
Collaborator

No description provided.

@mark-koch mark-koch requested a review from a team as a code owner October 29, 2024 16:00
@mark-koch mark-koch requested review from doug-q and removed request for a team October 29, 2024 16:00
Comment on lines -58 to +57
raise GuppyError("Guppy functions can only be called in a Guppy context")
raise RuntimeError("Guppy functions can only be called in a Guppy context")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Guppy errors are now reserved for things that go wrong during compilation. This error is thrown immediately when the user calls the function, so we shouldn't use GuppyError

@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 97.05882% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.40%. Comparing base (16d84b5) to head (a7f6aa1).
Report is 5 commits behind head on feat/diagnostics.

Files with missing lines Patch % Lines
guppylang/definition/struct.py 95.34% 2 Missing ⚠️
guppylang/definition/function.py 80.00% 1 Missing ⚠️
guppylang/definition/value.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                  @@
##           feat/diagnostics     #604      +/-   ##
====================================================
+ Coverage             91.75%   92.40%   +0.65%     
====================================================
  Files                    61       65       +4     
  Lines                  6633     7162     +529     
====================================================
+ Hits                   6086     6618     +532     
+ Misses                  547      544       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@doug-q doug-q 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, a couple of error paths missing coverage, I'm ok with them if you are.

self.defined_at,
)
err = NoSignatureError(self.defined_at, self.name)
err.add_sub_diagnostic(NoSignatureError.Suggestion(None))
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it uses the default message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The None means that this sub diagnostic does not have span, it's just rendered as text below the main error.

I updated to code so that this subdiagnostic is added by default

guppylang/definition/function.py Show resolved Hide resolved
sources.add_file(file)
annotate_location(func_ast, source, file, line_offset)
if not isinstance(func_ast, ast.FunctionDef):
raise GuppyError("Expected a function definition", func_ast)
raise GuppyError(ExpectedError(func_ast, "a function definition"))
Copy link
Contributor

Choose a reason for hiding this comment

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

no coverage, ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a test 👍

@@ -69,6 +101,8 @@ def parse(self, globals: "Globals", sources: SourceMap) -> "CustomFunctionDef":
code. The only information we need to access is that it's a function type and
that there are no unsolved existential vars.
"""
if not has_empty_body(self.defined_at):
raise GuppyError(BodyNotEmptyError(self.defined_at.body[0], self.name))
Copy link
Contributor

Choose a reason for hiding this comment

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

no coverage, ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a test 👍

@@ -245,7 +271,7 @@ def parse_py_class(cls: type, sources: SourceMap) -> ast.ClassDef:
if defn is not None:
annotate_location(defn.node, defn.cell_source, f"<{defn.cell_name}>", 1)
if not isinstance(defn.node, ast.ClassDef):
raise GuppyError("Expected a class definition", defn.node)
raise GuppyError(ExpectedError(defn.node, "a class definition"))
Copy link
Contributor

Choose a reason for hiding this comment

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

no coverage, ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is only triggered in notebook so makes it hard to test. I'll leave it without coverage for now

@@ -254,12 +280,12 @@ def parse_py_class(cls: type, sources: SourceMap) -> ast.ClassDef:
cls_ast = ast.parse(source).body[0]
file = inspect.getsourcefile(cls)
if file is None:
raise GuppyError("Couldn't determine source file for class")
raise GuppyError(UnknownSourceError(None, cls))
Copy link
Contributor

Choose a reason for hiding this comment

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

no coverage

# Store the source file in our cache
sources.add_file(file)
annotate_location(cls_ast, source, file, line_offset)
if not isinstance(cls_ast, ast.ClassDef):
raise GuppyError("Expected a class definition", cls_ast)
raise GuppyError(ExpectedError(cls_ast, "a class definition"))
Copy link
Contributor

Choose a reason for hiding this comment

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

no coverage

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a test 👍

raise GuppyError(
f"Parameter `{node.id}` cannot be used multiple times", node
)
raise GuppyError(RepeatedTypeParamError(node, node.id))
Copy link
Contributor

Choose a reason for hiding this comment

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

no coverage

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a test 👍

13 | def x(self: "MyStruct") -> int:
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
14 | return 0
| ^^^^^^^^^^^^^^^^ Struct `MyStruct` already contains a field named `x`
Copy link
Contributor

Choose a reason for hiding this comment

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

what's going on with this ^^^.. prefix, looks wrong

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It highlights the whole definition span, including the leading whitespace (since this method is indented inside a class)

@mark-koch mark-koch merged commit f7a0cdc into feat/diagnostics Nov 11, 2024
2 checks passed
@mark-koch mark-koch deleted the diag/defs branch November 11, 2024 13:48
github-merge-queue bot pushed a commit that referenced this pull request Nov 11, 2024
This is the development branch for our new diagnostics infrastructure.
Tracked by #535.

*  #548
* #551
* #552 
* #553
* #586
* #588
* #587
* #589 
* #590
* #600
* #601
* #604 
* #605 
* #606
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.

3 participants