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

[Stan 2.33] Make expiring deprecations into errors #1287

Merged
merged 21 commits into from
Aug 15, 2023

Conversation

WardBrian
Copy link
Member

@WardBrian WardBrian commented Mar 9, 2023

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:

  • The old array syntax, e.g. int arr[5];. Use array[5] int arr; instead.
  • Distribution functions ending in _log. Use either _lpdf or _lpmf.
  • The functions binomial_coefficient_log, multiply_log, and cov_exp_quad. Use lchoose, lmultiply, and gp_exp_quad_cov respectively.
  • The if_else function. Use the ternary operator cond ? true_value : false_value
  • Use of CDFs with a , between the first and second argument. Use a |.
  • Comments beginning with #. Use //.
  • Use of <- for assignment. Use =.
  • The increment_log_prob function. Use the target += statement.
  • The get_lp() function. Use target().
  • The use of nested multi-indices on the left hand side of an assignment statement.

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)

Copy link
Collaborator

@nhuurre nhuurre left a comment

Choose a reason for hiding this comment

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

You should change Middle.Fun_kind.suffix_from_name too since it makes use of the deprecated _log suffix.

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?

src/frontend/Input_warnings.ml Outdated Show resolved Hide resolved
src/frontend/Semantic_error.ml Outdated Show resolved Hide resolved
src/frontend/Typechecker.ml Outdated Show resolved Hide resolved
src/middle/Utils.ml Outdated Show resolved Hide resolved
src/middle/Stan_math_signatures.ml Outdated Show resolved Hide resolved
src/stan_math_backend/Lower_expr.ml Outdated Show resolved Hide resolved
src/stanc/stanc.ml Outdated Show resolved Hide resolved
src/stancjs/stancjs.ml Outdated Show resolved Hide resolved
@WardBrian
Copy link
Member Author

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
Copy link
Collaborator

nhuurre commented Mar 10, 2023

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
Copy link
Member Author

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

@WardBrian
Copy link
Member Author

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

Copy link
Collaborator

@nhuurre nhuurre left a comment

Choose a reason for hiding this comment

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

Looks good.

@WardBrian WardBrian force-pushed the deprecations/2-32-removals branch from f935cc4 to fddefdd Compare April 3, 2023 18:25
@WardBrian WardBrian changed the title [Stan 2.32] Make expiring deprecations into errors [Stan 2.33] Make expiring deprecations into errors Apr 3, 2023
@WardBrian
Copy link
Member Author

Updating now following #1303

We will not merge this until during the 2.33 development cycle

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

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

❗ 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%) ⬆️
src/middle/Fun_kind.ml 92.30% <ø> (-1.64%) ⬇️
src/middle/Utils.ml 90.00% <ø> (-2.00%) ⬇️
src/frontend/Semantic_error.ml 91.58% <66.66%> (-0.46%) ⬇️
src/stanc/stanc.ml 82.14% <75.00%> (+0.58%) ⬆️
src/frontend/Deprecation_analysis.ml 88.23% <80.00%> (-4.15%) ⬇️
src/frontend/Typechecker.ml 89.62% <91.66%> (-1.30%) ⬇️
src/frontend/Deprecation_removals.ml 92.68% <92.68%> (ø)
src/frontend/SignatureMismatch.ml 81.89% <100.00%> (+0.77%) ⬆️

... and 6 files with indirect coverage changes

@WardBrian WardBrian marked this pull request as ready for review July 24, 2023 17:02
@WardBrian
Copy link
Member Author

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

Copy link
Collaborator

@nhuurre nhuurre left a comment

Choose a reason for hiding this comment

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

Still looks good.

@WardBrian WardBrian merged commit 0e32ba1 into master Aug 15, 2023
@WardBrian WardBrian deleted the deprecations/2-32-removals branch August 15, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants