Skip to content

Implement ThrowExpression's proposed by DIP 1034#451

Merged
WebFreak001 merged 1 commit intodlang-community:masterfrom
MoonlightSentinel:throw-exp
Jan 29, 2022
Merged

Implement ThrowExpression's proposed by DIP 1034#451
WebFreak001 merged 1 commit intodlang-community:masterfrom
MoonlightSentinel:throw-exp

Conversation

@MoonlightSentinel
Copy link
Contributor

ThrowExpression's replace ThrowStatement's which may appear nested into any other declaration.

src/dparse/ast.d Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what's the proper deprecation process is.
Couldn't mark ThrowStatement as deprecated due to this bug.

Copy link
Member

Choose a reason for hiding this comment

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

I think it might be best to remove/rename it completely (causing compiler errors) and have the new behavior already and do a version bump. Currently as I see it this change would mean ThrowStatement wouldn't be used at all anymore after some point, causing old code to still compile but no longer work as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what's the proper deprecation process is. Couldn't mark ThrowStatement as deprecated due to this bug.

I found where in dmd it does this check: https://github.com/dlang/dmd/blob/c2b23a3bc73cd8ef7559b86f77f7f9db7ccae507/src/dmd/dsymbolsem.d#L3815

Might take a bit to fix, but it looks pretty low hanging if you want to give it a go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[...] ThrowStatement wouldn't be used at all anymore after some point, causing old code to still compile but no longer work as expected.

The idea was to remove ThrowStatement once it's no longer parsed. Any program that has been adapted to ThrowExpression would keep working as expected and could simply remove the ThrowStatement-related code.

But outright removing it now should also be fine - assuming that users can likely transition by replacing ThrowStatement. with ThrowExpression.

Copy link
Member

Choose a reason for hiding this comment

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

if they dependend on ThrowStatement: (in a visitor)

  • if we keep it and also add ThrowExpression, the visit function is likely no longer going to be called as expected anymore or have some changes in accessed member properties (potentially causing silent breakage)
  • if we remove it and move all the relevant properties into ThrowExpression, the code will no longer compile and they can see that they need to update their code (loud breakage, no runnable wrong code)

So I think removing it is the better choice.

sadly I don't think we can alias ThrowStatement to some broken type to cause additional error messages, which would be optimal in guiding users how to upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old ThrowStatement is now an alias to ThrowExpression and the field names are now identical. So existing code referencing ThrowStatement should keep working with a deprecation message.

@MoonlightSentinel MoonlightSentinel marked this pull request as ready for review January 12, 2022 13:47
@MoonlightSentinel MoonlightSentinel force-pushed the throw-exp branch 2 times, most recently from d40ddbe to 33b9429 Compare January 14, 2022 11:51
@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #451 (33b9429) into master (ec921ba) will decrease coverage by 37.56%.
The diff coverage is 97.05%.

❗ Current head 33b9429 differs from pull request most recent head 6e92f3b. Consider uploading reports for the commit 6e92f3b to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master     #451       +/-   ##
===========================================
- Coverage   82.66%   45.09%   -37.57%     
===========================================
  Files          11       11               
  Lines        8280     8306       +26     
===========================================
- Hits         6845     3746     -3099     
- Misses       1435     4560     +3125     
Impacted Files Coverage Δ
src/dparse/astprinter.d 0.00% <ø> (-94.77%) ⬇️
src/dparse/formatter.d 39.36% <80.00%> (-3.75%) ⬇️
src/dparse/ast.d 67.13% <100.00%> (-2.89%) ⬇️
src/dparse/parser.d 42.69% <100.00%> (-48.55%) ⬇️
test/tester.d 0.00% <0.00%> (-99.66%) ⬇️
src/dparse/lexer.d 41.63% <0.00%> (-43.96%) ⬇️
src/dparse/stack_buffer.d 80.00% <0.00%> (-15.00%) ⬇️
src/std/experimental/lexer.d 79.48% <0.00%> (-5.13%) ⬇️
... and 2 more

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 ec921ba...6e92f3b. Read the comment docs.

Replaces `ThrowStatement`'s with `ThrowExpression`'s  which may appear
nested into any other declaration.
@MoonlightSentinel
Copy link
Contributor Author

Ping. The implementation & documentation was merged upstream

@WebFreak001 WebFreak001 merged commit 069a8f6 into dlang-community:master Jan 29, 2022
@MoonlightSentinel MoonlightSentinel deleted the throw-exp branch January 29, 2022 18:36
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.

3 participants

Comments