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: improve error types and messages by using thiserror #600

Merged
merged 119 commits into from
Apr 23, 2024

Conversation

colifran
Copy link
Contributor

@colifran colifran commented Mar 11, 2024

This PR introduces an improved mechanism for defining error types and associated error messages throughout the source code via the thiserror crate.

Previously, a single error named TransmuteError was defined. This error was implemented manually by implementing TransmuteError, From for TransmuteError, and Display for TransmuteError. TransmuteError was all encompassing for any transmutation related errors such as resource translation errors, parsing errors, import instruction errors, etc. Additionally, any use of the panic!, unimplemented!, or unreachable! macro will result in an error message that says "unreachable" in wasm assemblies.

In general, our current mechanism for defining error types and messages causes several problems:

  1. Our code is harder to debug since error types are all encompassing and too widely scoped
  2. Error messages are too generic when they originate from the panic!, unimplemented!, or unreachable! macros
  3. New custom error types must be implemented manually which slows down development speed and is error prone
  4. Users don't have clear error messages that help them to effectively debug resulting in a frustrating UX and increasing issues in our queue

Using the thiserror crate will eliminate these problems by allowing us to quickly define new error types that can be appropriately scoped by simply adding to the Error enum. The manual work of implementing new custom error types will be handled by the thiserror crate. Additionally, we can migrate away from the panic!, unimplemented!, or unreachable! macros to provide a better UX with improved error details.


In addition to the changes outlined above, this PR fixes 5 bugs found:

  1. The python synthesizer was using ! instead of not for Condition::Not
  2. The csharp synthesizer was using ! applied to the first operand for Condition::Not rather than applying ! to the entire statement wrapped in parentheses. This produces an error because ! is only valid for booleans, but applying ! to the first operand will result in instances where ! is applied to a string or number
  3. The python synthesizer was not converting operands to snake case for Condition::And
  4. The python synthesizer was not converting Condition::Not values to snake case
  5. The csharp synthesizer was using single quotes for strings to be split instead of double quotes which causes an error because it was expecting a single character

This PR refactored several areas in the csharp synthesizer that were not testable. The first area is the ResourceIr::Object arm when matching against ResourceIr. This branch had the same error case in multiple branches when matching against the ResourceIr::Object structure. As a result, only the first error could be tested for. A more correct implementation would be to factor this invalid structure out into a single arm that will throw an error if encountered.

Additionally, ConditionIr was implementing CsharpEmitter; however, this is no longer correct because ConditionIr does not throw any errors. This meant that the error case in each ConditionIr branch was unable to be tested.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
…t tests

Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
…ypescript, and java

Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
colifran and others added 18 commits March 26, 2024 09:02
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
@colifran colifran force-pushed the colifran/use-thiserror-crate branch from c7d4cca to 692696e Compare March 31, 2024 05:40
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
@cdklabs-automation cdklabs-automation added this pull request to the merge queue Apr 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 15, 2024
@TheRealAmazonKendra TheRealAmazonKendra added this pull request to the merge queue Apr 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 16, 2024
Copy link

codecov bot commented Apr 23, 2024

Codecov Report

Merging #600 (04e1ce5) into main (8b47ab3) will increase coverage by 4.2%.
Report is 1 commits behind head on main.
The diff coverage is 79.1%.

❗ Current head 04e1ce5 differs from pull request most recent head 41900cb. Consider uploading reports for the commit 41900cb to get more accurate results

Additional details and impacted files
Components Coverage Δ
Parser 78.2% <ø> (ø)
Intermediate Representation 90.6% <100.0%> (+8.1%) ⬆️
Synthesizers 92.0% <74.9%> (+4.2%) ⬆️
Other 30.2% <16.7%> (-5.8%) ⬇️
@@           Coverage Diff           @@
##            main    #600     +/-   ##
=======================================
+ Coverage   82.1%   86.3%   +4.2%     
=======================================
  Files         27      27             
  Lines       5097    5212    +115     
  Branches    5097    5212    +115     
=======================================
+ Hits        4186    4499    +313     
+ Misses       741     468    -273     
- Partials     170     245     +75     
Files Coverage Δ
src/errors/mod.rs 100.0% <100.0%> (ø)
src/ir/constructor/mod.rs 100.0% <ø> (+4.0%) ⬆️
src/ir/importer/mod.rs 100.0% <100.0%> (+8.1%) ⬆️
src/ir/mod.rs 94.2% <100.0%> (ø)
src/ir/outputs/mod.rs 90.0% <100.0%> (ø)
src/ir/resources/mod.rs 90.7% <100.0%> (+9.2%) ⬆️
src/ir/sub/mod.rs 95.5% <100.0%> (+4.3%) ⬆️
src/lib.rs 93.8% <ø> (ø)
src/synthesizer/mod.rs 100.0% <100.0%> (ø)
src/synthesizer/python/mod.rs 93.2% <90.9%> (+3.9%) ⬆️
... and 5 more

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99f513d...41900cb. Read the comment docs.

@TheRealAmazonKendra TheRealAmazonKendra added this pull request to the merge queue Apr 23, 2024
Merged via the queue into main with commit 4ed0696 Apr 23, 2024
5 checks passed
@TheRealAmazonKendra TheRealAmazonKendra deleted the colifran/use-thiserror-crate branch April 23, 2024 20:16
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

3 participants