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

Fixing small bugs uncovered by type checker #11422

Merged
merged 4 commits into from
Oct 31, 2024

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Oct 28, 2024

Pull Request Description

Once our libraries and tests are compiled with basic inference of method types, some warnings were reported:

X:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Database\0.0.0-dev\src\DB_Column.enso:1003:19: warning: Calling member method `div` on type Number will result in a No_Such_Method error in runtime.
 1003 |         halfway = scale.div 2
      |                   ^~~~~~~~~~~
X:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Image\0.0.0-dev\src\Matrix.enso:381:21: warning: Invoking a value that has a non-function type (type Image) will result in a Not_Invokable error in runtime.
  381 |     to_image self = Image (Image.from_vector self.normalize.to_vector self.rows self.channels)
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
X:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Table\0.0.0-dev\src\Internal\Multi_Value_Key.enso:94:22: warning: Calling static method `new` on (type Illegal_State) will result in a No_Such_Method error in runtime.
   94 |         Error.throw (Illegal_State.new "Ordered_Multi_Value_Key is not intended for usage in unordered collections.")
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This PR attempts to fix them.

There was also an expected error in the Examples:

X:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Examples\0.0.0-dev\src\Main.enso:139:36: warning: Calling static method `frobnicate` on (type No_Methods) will result in a No_Such_Method error in runtime.
  139 | no_such_method = Panic.recover Any No_Methods.frobnicate . catch
      |                                    ^~~~~~~~~~~~~~~~~~~~~

To avoid getting a warning, I wrapped this in (_ : Any) to make the warning go away. The behaviour of the function is unchanged.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@radeusgd radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Oct 28, 2024
@radeusgd radeusgd self-assigned this Oct 28, 2024
@radeusgd radeusgd marked this pull request as ready for review October 28, 2024 13:28
Comment on lines +1002 to +1005
Runtime.assert (decimal_places <= 0)
## We assert that the decimal_places is non-positive, so the scale will be an Integer.
We can tell that to the type-checker by adding an annotation.
scale = (10 ^ -decimal_places) : Integer
Copy link
Member Author

@radeusgd radeusgd Oct 28, 2024

Choose a reason for hiding this comment

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

This assert is documenting that this method will not work if decimal_places > 0. That is because then scale would be a Float which has no div method, so it would fail at runtime with No_Such_Method error. That's what the checker uncovered.

All tests were passing so it's either never called with decimal_places > 0 or our tests don't cover all the cases.

@GregoryTravis do you know if this method is ever expected to be called with decimal_places > 0? It was unclear to me looking at DB_Column.round and should_use_builtin_round if that is the case or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I must be missing some test case. Added #11424.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you

@radeusgd radeusgd force-pushed the wip/radeusgd/fix-bugs-found-by-checker branch from a15d803 to c81d863 Compare October 28, 2024 17:23
@radeusgd radeusgd force-pushed the wip/radeusgd/fix-bugs-found-by-checker branch from c81d863 to 68c05fc Compare October 28, 2024 17:31
@radeusgd radeusgd changed the base branch from wip/radeusgd/9812-infer-method-calls to develop October 28, 2024 17:31
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Oct 28, 2024
@mergify mergify bot merged commit cf5326f into develop Oct 31, 2024
32 of 36 checks passed
@mergify mergify bot deleted the wip/radeusgd/fix-bugs-found-by-checker branch October 31, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants