Add compiler warnings #837

2021-10-04T23:42:54+09:00

ghallak commented

2021-10-04 23:42:54 +09:00

(Migrated from gitlab.com)

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

Currently added warnings: - [x] shadowing - [x] negative spends - [x] division by zero - [x] unused functions - [x] unused includes - [x] unused `stateful` annotations - [x] unused variables - [x] unused parameters - [x] unused user-defined type - [x] dead return value - [ ] ~non-exhaustive patterns~ (this will be tricky and should better be done in a separate PR) Currently supported options: - [x] Turn options off/on separately. - [x] Warnings packages (Wall). pedantic package can easily be added later. - [x] Turn warnings into type errors. Things that still need to be done: - [x] Properly report the warnings on compilation. (Currently they are just stored but not reported) Testing: - [x] Tests for all added warnings - [x] Test for turning warnings into type errors

ghallak commented

2021-10-28 00:40:25 +09:00

(Migrated from gitlab.com)

@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?

@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?

zxq9 commented

2021-10-28 01:11:48 +09:00

(Migrated from gitlab.com)

Created by: radrow

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

*Created by: radrow* I think that warnings should be returned as renderable structures as a result of the compilation/infer function

zxq9 commented

2021-10-28 01:14:42 +09:00

(Migrated from gitlab.com)

Created by: hanssv

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.

*Created by: hanssv* 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.

zxq9 commented

2021-11-05 00:34:53 +09:00

(Migrated from gitlab.com)

Created by: dincho

(_"_"_) 🤷🏻‍♂️

*Created by: dincho* `(_"_"_)` 🤷🏻‍♂️

ghallak commented

2021-11-05 19:03:14 +09:00

(Migrated from gitlab.com)

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.

**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.

zxq9 commented

2021-11-06 01:59:23 +09:00

(Migrated from gitlab.com)

Created by: dincho

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

*Created by: dincho* > 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

zxq9 commented

2021-11-09 19:25:32 +09:00

(Migrated from gitlab.com)

Created by: radrow

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

*Created by: radrow* there could be a second pass of warning check after additional constant propagation

zxq9 commented

2021-11-11 17:48:52 +09:00

(Migrated from gitlab.com)

Created by: hanssv

Review: Approved

👍 Looks good!

*Created by: hanssv* **Review:** Approved 👍 Looks good!

zxq9 commented

2021-11-15 20:13:30 +09:00

(Migrated from gitlab.com)

Created by: radrow

Review: Approved

*Created by: radrow* **Review:** Approved

zxq9 commented

2021-11-15 20:14:05 +09:00

(Migrated from gitlab.com)

Created by: radrow

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

*Created by: radrow* Would be nice to reflect it in the cli and http interface too

zxq9 commented

2021-11-15 20:15:04 +09:00

(Migrated from gitlab.com)

Created by: radrow

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

*Created by: radrow* @nikita-fuchs would be great to see it working in aestudio :)

zxq9 commented

2021-11-15 20:31:13 +09:00

(Migrated from gitlab.com)

Created by: nikita-fuchs

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

dayumn, thanks for bringing this up.

*Created by: nikita-fuchs* > @nikita-fuchs would be great to see it working in aestudio :) dayumn, thanks for bringing this up.

zxq9 commented

2021-11-24 18:46:22 +09:00

(Migrated from gitlab.com)

Merged by: ghallak at 2021-11-24 09:46:22 UTC

*Merged by: ghallak at 2021-11-24 09:46:22 UTC*

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: QPQ-AG/sophia#837