stan-dev / stanc3

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

Include function suffixes when looking for "Did you mean" suggestions #1401

Closed nhuurre closed 7 months ago

nhuurre commented 7 months ago

Submission Checklist

Since #1024, unknown identifier related errors give a similar identifier as a suggestion but the algorithm only considered typos involving at most three letters. Another likely error is omitting the distribution suffix from a function name: https://discourse.mc-stan.org/t/how-to-interpret-a-returning-function-was-expected-but-an-undeclared-identifier-was-supplied/34078

Release notes

When encountering an unknown identifier that matches a known suffixed function without its suffix, add the known function name to the error message.

Copyright and Licensing

Copyright holder: Niko Huurre

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)

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (35c682b) 89.88% compared to head (ddacb1a) 89.88%. Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1401 +/- ## ======================================= Coverage 89.88% 89.88% ======================================= Files 63 63 Lines 10508 10513 +5 ======================================= + Hits 9445 9450 +5 Misses 1063 1063 ``` | [Files](https://app.codecov.io/gh/stan-dev/stanc3/pull/1401?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stan-dev) | Coverage Δ | | |---|---|---| | [src/frontend/Environment.ml](https://app.codecov.io/gh/stan-dev/stanc3/pull/1401?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stan-dev#diff-c3JjL2Zyb250ZW5kL0Vudmlyb25tZW50Lm1s) | `69.49% <100.00%> (+2.82%)` | :arrow_up: |
WardBrian commented 7 months ago

This is going to end up being based on the order of the lists of suffixes in Utils, right? E.g. if a lpdf is available, it will never suggest lccdf

That’s already an improvement so I’m happy to merge as is, but how would you feel about giving a list of suggestions?

nhuurre commented 7 months ago

If any of them matches then almost all of them are going to match, and I think that's excessive. One option would be to omit _*cdfs and choose either _lpdf/_lpmf or _rng based on the argument types but that adds some complexity.