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

Preventing compilation of a @tailrec method when it does not rewrite, but an inner method does #20143

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

LucySMartin
Copy link

Adding warnings if there is an annotated def at the top level that is referenced from an inner def

Fixes #20105

@som-snytt
Copy link
Contributor

som-snytt commented Apr 9, 2024

Sorry, I complained about having to rewrite error IDs repeatedly. Probably there is a way to make that part of the build. Or maybe they use it as a rite of passage of sorts.

@LucySMartin
Copy link
Author

Sorry, I complained about having to rewrite error IDs repeatedly.

No worries. I've had much much worse hand numbered files to merge in my time.

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Looks reasonable overall. Missing check files + some minor suggestions.

tests/neg/i20105.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/transform/TailRec.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/reporting/messages.scala Outdated Show resolved Hide resolved
tests/warn/i20105.scala Outdated Show resolved Hide resolved
@LucySMartin LucySMartin marked this pull request as ready for review April 12, 2024 12:02
@sjrd
Copy link
Member

sjrd commented Jun 10, 2024

@LucySMartin What's the status of this PR? Do you need help getting the CI to pass?

@LucySMartin
Copy link
Author

LucySMartin commented Jun 10, 2024 via email

@LucySMartin
Copy link
Author

Please ignore the excess off way too many commits. Was fighting either git or the build as it kept putting /weird/ CR and LF tokens into things.
I know Ill need to squash it.

@LucySMartin
Copy link
Author

@sjrd - I'm really sorry but I've given this a few attempts to get line endings correct, but its just not working on the remote.

Are there some standard git settings or arcane windows magic I would need to do to get these things to generate with the correct line endings?

@sjrd
Copy link
Member

sjrd commented Jun 17, 2024

On Windows I strongly suggest disabling the "autocrlf" config. I even recommend to that globally. git pretends to be "helpful" with that setting but I have never seen it useful and always harmful.

To disable it:

$ git config --global core.autocrlf false

taken from the git book

@som-snytt
Copy link
Contributor

Offering sympathies. I never develop on Windows, but a fellow at the office did and we went through this rigmarole.

I just read that the U.S. Surgeon General will require a warning on the package for autocrlf.

@LucySMartin
Copy link
Author

Yeah - I turned autocrlf off for the reattempt today (and have nuked it on other projects too - I agree from the docs its just bad).
They then generated via sbt with the wonderful line endings of CR-CR-LF.

I saw that comparable check files were just LF (please correct me if that's not right!) so stripped the files down to just LF by hand, and copied them across.

I'm honestly not sure what is wrong with them, and (sorry if this is a problem with my limited use of github) I cant see any way to view what files were actually generated via the CI tests, to validate what line endings its generated in the server side .out files.

@LucySMartin
Copy link
Author

Im an idiot - I see the issue.

Copy link
Author

@LucySMartin LucySMartin Jun 17, 2024

Choose a reason for hiding this comment

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

Note to self: drop this when sqashing commits.

Copy link
Member

@sjrd sjrd 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, thank you :)

I'll let you squash in order to have a single commit with a clean commit message. Other than that it will be good to go.

…ons contain non-tail recursive calls.

Code will now compile where a child def calls the parent def in a non-tail position (with the warning).
Code will no longer compile if all calls to a @tailrec method are in named child methods (as these do not tail recurse).
@sjrd
Copy link
Member

sjrd commented Jun 19, 2024

Thanks. Is it intended that the commit is not linked to your GitHub account? That's not a problem for us, but it means you won't get the statistics in your favor. :p

@LucySMartin
Copy link
Author

Note in addition to the squash I removed the hex-logging of the comparison.
I might try and make that error message slightly more user friendly at some point, but that's a super low priority.

@LucySMartin
Copy link
Author

Anything I need to do to merge this at this point?
I have another little fix that is going to conflict with this one, would be good to get it in while I still have some downtime.

@sjrd sjrd merged commit eb2ea26 into scala:main Jun 19, 2024
24 checks passed
@LucySMartin LucySMartin deleted the nested-tailrec-defs branch June 19, 2024 14:26
@som-snytt
Copy link
Contributor

som-snytt commented Jun 24, 2024

In case sjrd's comment was overlooked, the commit author is

Author: Lucy Martin <lucy.martin@nttdata.com>

I had that issue once, but I don't remember how I managed my .gitconfig, maybe with a project-specific one?

(I was visiting this PR in order to align Scala 2.)

@LucySMartin
Copy link
Author

Oh oops.
While I don't have any real attachment to my github stats (heck this is a brand new account) I really should be being a lot more careful with that.
Thanks for the call out.

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.

@tailrec and nested definitions
4 participants