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: Improved error message for unexpected return type #2302

Merged
merged 3 commits into from Aug 15, 2023
Merged

feat: Improved error message for unexpected return type #2302

merged 3 commits into from Aug 15, 2023

Conversation

ghost
Copy link

@ghost ghost commented Aug 14, 2023

Description

better return type error message

Problem

Resolves #2105

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@kevaundray
Copy link
Contributor

For totality, can you provide images of the before and after?

@ghost
Copy link
Author

ghost commented Aug 14, 2023

@kevaundray

before:

error: Expected type Field, found type ()
  ┌─ /home/work/subject/oliver/noir/h/src/main.nr:1:4
  │
1 │ fn main() -> Field {
  │    ----

Error: Aborting due to 2 previous errors

after (empty function):

error: Expected return type Field, but a value of type () was actually returned
  ┌─ /home/work/subject/oliver/noir/h/src/main.nr:1:4
  │
1 │ fn main() -> Field {}
  │    ----
  │
  = implicitly returns `()` as its body has no tail or `return` expression

after (non-empty function):

error: Expected return type Field, but a value of type () was actually returned
  ┌─ /home/work/subject/oliver/noir/h/src/main.nr:1:4
  │
1 │ fn main() -> Field {
  │    ----

Perhaps it's necessary to store a span for the return type and point it specifically to this location, rather than just the function name.

@ghost
Copy link
Author

ghost commented Aug 14, 2023

for comparison with Rust:

noir:

error: Expected return type (), but a value of type Field was actually returned
  ┌─ /home/work/subject/oliver/noir/h/src/main.nr:1:4
  │
1 │ fn main() -> () {
  │    ----

rust:

 --> src/lib.rs:2:5
  |
1 | fn lotsofRead() -> () {
  |                    -- expected `()` because of return type
2 |     42
  |     ^^ expected `()`, found integer

probably need something like this

@jfecher
Copy link
Contributor

jfecher commented Aug 14, 2023

Perhaps it's necessary to store a span for the return type and point it specifically to this location, rather than just the function name.

Can you include that as part of this PR? Where this error message is generated we should have access to the function body which you can match on and get the location of the last expression if it is a block expression, or just get the location of the overall body if not.

@kevaundray
Copy link
Contributor

@f01dab1e can you give an updated before and after with the return type span?

@ghost
Copy link
Author

ghost commented Aug 15, 2023

@kevaundray

error: expected type (), found type Field
  ┌─ /home/work/subject/oliver/noir/h/src/main.nr:3:11
  │
3 │ fn main() {
  │           - expected () because of return type
4 │     2
  │     - expected type (), found type Field
  │
  = help: try adding a return type: `-> Field`

Error: Aborting due to 1 previous error
error: expected type (), found type Field
  ┌─ /home/work/subject/oliver/noir/h/src/main.nr:3:14
  │
3 │ fn main() -> () {
  │              -- expected () because of return type
4 │     2
  │     - expected type (), found type Field

Error: Aborting due to 1 previous error
error: expected type Field, found type ()
  ┌─ /home/work/subject/oliver/noir/h/src/main.nr:3:14
  │
3 │ fn main() -> Field {
  │              ----- expected Field because of return type
  │
  = implicitly returns `()` as its body has no tail or `return` expression

crates/noirc_frontend/src/ast/expression.rs Show resolved Hide resolved
crates/noirc_frontend/src/hir_def/function.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir/type_check/errors.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir/type_check/mod.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir/type_check/mod.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/parser/parser.rs Outdated Show resolved Hide resolved
@jfecher
Copy link
Contributor

jfecher commented Aug 15, 2023

Some very good improvements to the error messages here

@jfecher jfecher added this pull request to the merge queue Aug 15, 2023
@TomAFrench TomAFrench changed the title chore: better return type error message chore: better error message for unexpected return type Aug 15, 2023
@TomAFrench TomAFrench changed the title chore: better error message for unexpected return type feat: Improved error message for unexpected return type Aug 15, 2023
Merged via the queue into noir-lang:master with commit d7e1e65 Aug 15, 2023
19 checks passed
TomAFrench added a commit that referenced this pull request Aug 15, 2023
* master:
  feat: Improved error message for unexpected return type (#2302)
  feat(stdlib): Implement `str` `as_bytes` and `into_bytes` function (#2298)
  chore(ci): automatically convert changelog entries to sentence case (#2325)
TomAFrench added a commit that referenced this pull request Aug 16, 2023
* master: (34 commits)
  chore: move orphaned integration tests to new directory (#2331)
  chore(noir): Release 0.10.1 (#2328)
  feat(ssa): Switch mem2reg pass to be per function rather than per block (#2243)
  feat(ssa): Perform dead instruction elimination on intrinsic functions (#2276)
  feat: Add full call stacks to runtime errors (#2310)
  chore(ci): fix mismatched input name to publish workflow (#2327)
  chore: add README for integration test structure (#2277)
  feat: Improved error message for unexpected return type (#2302)
  feat(stdlib): Implement `str` `as_bytes` and `into_bytes` function (#2298)
  chore(ci): automatically convert changelog entries to sentence case (#2325)
  chore(noir): Release 0.10.0 (#2039)
  fix(lsp): Ensure lsp does not crawl past the root specified (#2322)
  fix: Prevent panic when passing relative paths to `--program-dir` (#2324)
  fix: Overflowing assignment will result in an error (#2321)
  chore: clippy fixes (#2320)
  chore: Parameterize the build mode for noir-wasm (#2317)
  chore: Make `wasm` tests pull from `result` directory (#2319)
  chore: Fix typo (#2315)
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  ...
TomAFrench added a commit that referenced this pull request Aug 17, 2023
* master: (25 commits)
  chore: update noir-source-resolver from `1.1.2` to `^1.1.3` (#2349)
  chore(ci): Avoid writing to cache in workflows triggered by the merge queue (#2341)
  chore(noir): Release 0.10.3 (#2344)
  feat(lsp): Add `Execute` code lens for `main` functions (#2330)
  feat(lsp): Add `Compile` code lens for `main` function and contracts (#2309)
  feat: Allow calling higher-order functions with closures (#2335)
  fix: Display warning if last expression of block is unused (#2314)
  chore(noir): Release 0.10.2 (#2343)
  fix: Prevent dead instruction elimination of brillig functions which may contain side-effects (#2340)
  chore: Separate integration tests for contracts and programs (#2339)
  chore: move orphaned integration tests to new directory (#2331)
  chore(noir): Release 0.10.1 (#2328)
  feat(ssa): Switch mem2reg pass to be per function rather than per block (#2243)
  feat(ssa): Perform dead instruction elimination on intrinsic functions (#2276)
  feat: Add full call stacks to runtime errors (#2310)
  chore(ci): fix mismatched input name to publish workflow (#2327)
  chore: add README for integration test structure (#2277)
  feat: Improved error message for unexpected return type (#2302)
  feat(stdlib): Implement `str` `as_bytes` and `into_bytes` function (#2298)
  chore(ci): automatically convert changelog entries to sentence case (#2325)
  ...
@ghost ghost mentioned this pull request Aug 22, 2023
2 tasks
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.

Better return type error message
2 participants