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 compiler warnings #346

Merged
merged 2 commits into from
Nov 24, 2021
Merged

Add compiler warnings #346

merged 2 commits into from
Nov 24, 2021

Conversation

ghallak
Copy link
Contributor

@ghallak ghallak commented Oct 4, 2021

Currently added warnings:

  • shadowing
  • negative spends
  • division by zero
  • unused functions
  • unused includes
  • unused stateful annotations
  • unused variables
  • unused parameters
  • unused user-defined type
  • dead return value
  • non-exhaustive patterns (this will be tricky and should better be done in a separate PR)

Currently supported options:

  • Turn options off/on separately.
  • Warnings packages (Wall). pedantic package can easily be added later.
  • Turn warnings into type errors.

Things that still need to be done:

  • Properly report the warnings on compilation. (Currently they are just stored but not reported)

Testing:

  • Tests for all added warnings
  • Test for turning warnings into type errors

@ghallak
Copy link
Contributor Author

ghallak commented Oct 27, 2021

@hanssv @radrow I'm looking for a way to properly report the warnings. Right now, the warnings are all stored in an ets_table called warnings in the aeso_ast_infer_types.erl file.

Do you think I should keep forwarding the warnings as strings upwards until they get to the aeso_compiler:from_string function and then I can return them along with the compilation result? or is there another way to do that?

@radrow
Copy link
Member

radrow commented Oct 27, 2021

I think that warnings should be returned as renderable structures as a result of the compilation/infer function

@hanssv
Copy link
Member

hanssv commented Oct 27, 2021

Yes, keeping them in some warning structure (similar to the errors) and passing them upwards sounds like the best alternative. Then we can present them, or return them (or ignore them) according to options I guess.

@ghallak ghallak marked this pull request as ready for review November 5, 2021 09:34
Add include_type annotation to position

Add warning for unused includes

Add warning for unused stateful annotation

Add warning for unused functions

Add warning for shadowed variables

Add division by zero warning

Add warning for negative spends

Add warning for unused variables

Add warning for unused parameters

Change the ets table type to set for unused vars

Add warning for unused type defs

Move unused variables warning to the top level

Temporarily disable unused functions warnings

Add all kinds of warnings to a single ets table

Enable warnings separately through options

Use when_option instead of enabled_warnings

Turn warnings into type errors with warn_error option

Enable warning package warn_all

Re-enable unused functions warnings

Report warnings as type errors in a separate function

Make unused_function a recognized warning

Report warnings as a result of compilation

Fix tests and error for unknown warnings options

Fix dialyzer warnings

Do not show warning for variables called "_"

Move warnings handling into a separate module

Do not show warning for unused public functions in namespaces

Add src file name to unused include warning

Mark public functions in namespaces as used

Add tests for added warnings

Add warning for unused return value

Add test for turning warnings into type errors
@ghallak
Copy link
Contributor Author

ghallak commented Nov 5, 2021

For negative spends and division by zero, the warning will only show when using a literal.

function f() =
  let x = 1 / 0  // this is a warning
  
  let zero = 0
  let y = 1 / zero  // no warning will show here

  let z = 1 / (1 - 1)  // no warning will show here

I think it won't be very difficult to add warning for the second case 1 / zero since this will only need to lookup the value of the variables zero, but the third case will requires evaluating the expression 1 - 1, which will make things more complicated and I'm not sure if I should try to add that.

@dincho
Copy link
Member

dincho commented Nov 5, 2021

but the third case will requires evaluating the expression 1 - 1, which will make things more complicated and I'm not sure if I should try to add that.

I guess if the compiler have a optimization phase to evaluate that and you run the warning check in a step/phase after it it would be covered by case 1

@radrow
Copy link
Member

radrow commented Nov 9, 2021

there could be a second pass of warning check after additional constant propagation

Copy link
Member

@hanssv hanssv 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!

@radrow
Copy link
Member

radrow commented Nov 15, 2021

Would be nice to reflect it in the cli and http interface too

@radrow
Copy link
Member

radrow commented Nov 15, 2021

@nikita-fuchs would be great to see it working in aestudio :)

@nikita-fuchs
Copy link
Contributor

@nikita-fuchs would be great to see it working in aestudio :)

dayumn, thanks for bringing this up.

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.

None yet

5 participants