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

fix: never return type in let initializer #264

Merged
merged 1 commit into from
Sep 6, 2020

Conversation

baszalmstra
Copy link
Collaborator

@baszalmstra baszalmstra commented Sep 5, 2020

Solves an issue where if an initializer of a let statement has the never type it crashed the code generation.

Fixes #262

@codecov
Copy link

codecov bot commented Sep 5, 2020

Codecov Report

Merging #264 into master will increase coverage by 0.37%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
+ Coverage   78.75%   79.13%   +0.37%     
==========================================
  Files         215      214       -1     
  Lines       12857    12794      -63     
==========================================
- Hits        10125    10124       -1     
+ Misses       2732     2670      -62     
Impacted Files Coverage Δ
crates/mun_codegen/src/ir/body.rs 84.23% <80.00%> (-0.14%) ⬇️
crates/mun_codegen/src/test.rs 96.58% <100.00%> (+0.02%) ⬆️
crates/tools/src/lib.rs 70.96% <0.00%> (-1.76%) ⬇️
crates/mun_language_server/src/main_loop.rs 23.21% <0.00%> (-0.35%) ⬇️
crates/mun/src/ops/init.rs 100.00% <0.00%> (ø)
crates/mun_codegen_macros/src/lib.rs
crates/mun_compiler/src/driver.rs 59.76% <0.00%> (+0.11%) ⬆️
crates/mun_target/src/abi/mod.rs 57.28% <0.00%> (+0.55%) ⬆️
crates/mun_libloader/src/lib.rs 73.91% <0.00%> (+1.91%) ⬆️
crates/mun_codegen/src/linker.rs 25.33% <0.00%> (+2.98%) ⬆️

Continue to review full report at Codecov.

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

Copy link
Collaborator

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

The fix looks good. I just have one question w.r.t. making the user aware of dead code. In the future we could potentially be dealing with side-effects, so we can do a similar recommendation to Rust. (i.e. use an _before_var_name if you want the dead code to not raise a warning)

self.gen_let_statement(*pat, *initializer);
// If the let statement never finishes, there is no need to generate more code
if !self.gen_let_statement(*pat, *initializer) {
return None;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a dead_code diagnostic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes we should add that at some point but thats beyond the scope of this PR

Copy link
Collaborator

@Wodann Wodann Sep 6, 2020

Choose a reason for hiding this comment

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

Is it though? I'm talking about at least adding the diagnostic

Adding hard opt-outs without giving any user feedback reduces usability. Moving this to a potential future improvement generally results in a large degradation before it's tackled (or even gets forgotten). It seems like this is the best time to also do that additional small change, instead of having inconveniences accumulate over time.

@Wodann Wodann merged commit 5b80df4 into mun-lang:master Sep 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: fix Bug fix or report
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'main' panicked at 'internal error: entered unreachable code
2 participants