stan-dev / stanc3

The Stan transpiler (from Stan to C++ and beyond).
BSD 3-Clause "New" or "Revised" License
138 stars 44 forks source link

[Stan 2.33] Make expiring deprecations into errors #1287

Closed WardBrian closed 10 months ago

WardBrian commented 1 year ago

This adds some (primarily temporary) code which is modeled off the current Deprecation_analysis model but is used to generate errors rather than warnings.

Submission Checklist

Release notes

The following deprecations have been turned into errors this version:

For this version, these can all be automatically updated with the --canonicalize=deprecations argument to the autoformatter. This is not guaranteed to work for versions following this one.

Additionally, the following identifiers are now reserved words: array, offset, multiplier, lower, and upper.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

WardBrian commented 1 year ago

The new Deprecation_removals.ml is mostly a copy of Deprecation_analysis.ml, right? What's your opinion on keeping them together, like, instead of Deprecation_analysis.collect_warnings you'd have Deprecation_analysis.collect_warnings_and_removals that returns two lists?

I like having it separate because I'm optimizing this code for deletion. It would be nice to combine logic (like for deprecated functions, currently the way this works is basically like having an if with an else in a different module), but the idea is that most of this module will only exist for one version.

If you feel strongly about it I can merge the two - would you want the functions themselves to be merged (so we only walk the AST once) or just move over the contents of the removal module and define collect_warnings_and_removals such that it calls both?

nhuurre commented 1 year ago

I like having it separate because I'm optimizing this code for deletion ... the idea is that most of this module will only exist for one version.

In that case it makes sense to keep them separate but I'd consider re-integrating them in the next version.

WardBrian commented 1 year ago

Thanks @nhuurre. I'm going to wait to merge until I write the documentation PR early next week

WardBrian commented 1 year ago

@nhuurre while writing up the doc I realized the nested lvalue issue is also expiring. Care to take one last glance?

WardBrian commented 1 year ago

Updating now following #1303

We will not merge this until during the 2.33 development cycle

codecov[bot] commented 1 year ago

Codecov Report

Merging #1287 (a15b411) into master (0f36ee7) will decrease coverage by 0.20%. The diff coverage is 89.74%.

:exclamation: Current head a15b411 differs from pull request most recent head 27a6ed8. Consider uploading reports for the commit 27a6ed8 to get more accurate results

@@            Coverage Diff             @@
##           master    #1287      +/-   ##
==========================================
- Coverage   89.32%   89.13%   -0.20%     
==========================================
  Files          64       65       +1     
  Lines       10588    10632      +44     
==========================================
+ Hits         9458     9477      +19     
- Misses       1130     1155      +25     
Files Changed Coverage Δ
src/frontend/Input_warnings.ml 100.00% <ø> (+17.64%) :arrow_up:
src/middle/Fun_kind.ml 92.30% <ø> (-1.64%) :arrow_down:
src/middle/Utils.ml 90.00% <ø> (-2.00%) :arrow_down:
src/frontend/Semantic_error.ml 91.58% <66.66%> (-0.46%) :arrow_down:
src/stanc/stanc.ml 82.14% <75.00%> (+0.58%) :arrow_up:
src/frontend/Deprecation_analysis.ml 88.23% <80.00%> (-4.15%) :arrow_down:
src/frontend/Typechecker.ml 89.62% <91.66%> (-1.30%) :arrow_down:
src/frontend/Deprecation_removals.ml 92.68% <92.68%> (ø)
src/frontend/SignatureMismatch.ml 81.89% <100.00%> (+0.77%) :arrow_up:

... and 6 files with indirect coverage changes

WardBrian commented 10 months ago

@nhuurre mind taking another look at this after the various merges that have been necessary?