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.

Sign up for GitHub

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

Jump to bottom

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
Merged

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

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 som-snytt mentioned this pull request Apr 9, 2024
Merged
@LucySMartin LucySMartin mentioned this pull request Apr 9, 2024
Closed
@LucySMartin LucySMartin marked this pull request as draft April 9, 2024 20:24
@som-snytt
Copy link
Contributor

som-snytt commented Apr 9, 2024 •
edited
Loading

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.

@LucySMartin LucySMartin force-pushed the nested-tailrec-defs branch from f510492 to 73aae73 Compare April 10, 2024 09:24
@Gedochao Gedochao requested a review from sjrd April 10, 2024 10:56
sjrd
sjrd requested changes Apr 10, 2024
View reviewed changes
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/transform/TailRec.scala 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 force-pushed the nested-tailrec-defs branch from dea5b00 to a841499 Compare April 12, 2024 12:02
@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.

LucySMartin
LucySMartin commented Jun 17, 2024
View reviewed changes
compiler/test/dotty/tools/vulpix/FileDiff.scala Outdated
Copy link
Author

@LucySMartin LucySMartin Jun 17, 2024 •
edited
Loading

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.

sjrd
sjrd reviewed Jun 18, 2024
View reviewed changes
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.

scala#20105: Adding a warning to the case where nested named definiti…
01ada74 
…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).
@LucySMartin LucySMartin force-pushed the nested-tailrec-defs branch from 04abdf0 to 01ada74 Compare June 19, 2024 07:46
@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 •
edited
Loading

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

Author: Lucy Martin <[email protected]>

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.

@Kordyjan Kordyjan added this to the 3.5.1 milestone Jul 3, 2024
@WojciechMazur WojciechMazur mentioned this pull request Jul 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjrd sjrd sjrd approved these changes

Assignees

@sjrd sjrd

Labels
None yet
Projects
None yet
Milestone
3.5.1
Development

Successfully merging this pull request may close these issues.

@tailrec and nested definitions
4 participants
@LucySMartin @som-snytt @sjrd @Kordyjan