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

[WIP] Tail recursion warning and attribute #1976

Closed
wants to merge 11 commits into from

Conversation

AviAvni
Copy link
Contributor

@AviAvni AviAvni commented Dec 9, 2016

Start working on fsharp/fslang-suggestions#147

  • add tests
  • check for async { .. } and seq { .. }

image

@vasily-kirichenko
Copy link
Contributor

TailRecursionAttribute is too long for my taste. What about TailrecAttribute?
Scala uses @tailrec attribute.

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Dec 9, 2016

This is a wonderful addition to F#! Writing either [<TailRec>] or [<TailRecursion>] wouldn't matter much to me (@vasily-kirichenko). In fact, I think if at all possible, let rec f vs let tail rec f (or let tailrec f) would have my preference, but that'd mean introducing a new keyword.

We may need a keyword if we are to allow this feature on local functions:

let factorial n =
    let rec loop i acc =   // this is tail-recursive, but if it isn't I'd like the new warning
        match i with
        | 0 | 1 -> acc
        | _ -> loop (i-1) (acc * i)
    loop n 1

@forki
Copy link
Contributor

forki commented Dec 10, 2016

Mhm since many many rec functions are local I think the introduction of a new tailrec keyword makes a lot of sense. If we decide on attributes then we should go with "TailRecursionAttribute". This is very similar to the existing RequireQualifiedAccessAttribute".

Regarding the error message. I think we should make it really verbose and explain as much as possible.

@forki
Copy link
Contributor

forki commented Dec 10, 2016

Forgot to mention that this is just wonderful. One of the best additions to the compiler in quite some time. Kudos for tackling this.

@forki
Copy link
Contributor

forki commented Dec 10, 2016

There is also the possibility of taking a more radical approach and to always check for tail recursion. This has a couple of advantages.

  1. we don't need to introduce new syntax constructs nor attributes
  2. existing code will be checked without the need of change
  3. people can always opt-out by nowarn directive - that's already built in
  4. using tail recursion is whenever it is possible also the best way to do things and should therefore the default choice of the language
  5. it is still non-breaking (ignoring "warning as error" mode like we usually do)

Tbh I think this should be really considered. @dsyme what do you think?

@thinkbeforecoding
Copy link
Contributor

@forki suggestion to check tail recursion systematically seems a good idea.
Recursion is to my experience really often used in local nested functions. So it'd be a problem if attributes are limited to top level functions (didn't check).
There's a tailcall reserved keyword, but it seemed @dsyme didn't recommend its usage.

@vilinski
Copy link

+1 to check always

@AviAvni
Copy link
Contributor Author

AviAvni commented Dec 10, 2016

Thanks for everyone I will wait for @dsyme to decide about the details

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Dec 10, 2016

I'm in favor of checking always, with one caveat: beginning users will be stymied by this warning, it requires a working knowledge of too many concepts to even understand the gist of what it means, let alone knowing how to fix it.

Either we should make it a L4 warning (are levels currently used?) or we should make sure that the warning includes a "ignore me" statement, for instance:

FS8888: This function is marked recursive, but is not tail-recursive. If the recursion is deep or performance is of the essence, consider making it tail-recursive, otherwise, ignore this warning with #nowarn 8888.

Either way, I would prefer this as an option for experienced programmers. In 9 out of 10 cases it doesn't matter, as recursion won't go too deep anyway and performance isn't critical. If L3 warnings is the default, making it an L4 warning would suffice (though I am uncertain whether or not the levels are actually adhered to, or used, see for instance this uservoice post, which wasn't converted?)

Warnings like these should either be leveled (L4, L5), or be allowed to be switched on/off on a more granular level than per-file (but that's OT for this issue, I know).

Even better yet: if someone feels like it, this could be switched on/off on the property page of a project (an option "analyze for tail-recursion").

@vladima
Copy link
Contributor

vladima commented Dec 10, 2016

Idea of detecting calls in non-tail position definitely sounds useful, however I think that implementation should be different. Reason: currently it is done at the codegen phase which is IDE never does (because there is no point to generate actual binary on every keystroke) - so this error will never be reported in editor which to me significantly decreases its value.

@forki
Copy link
Contributor

forki commented Dec 10, 2016

In 9 out of 10 cases it doesn't matter, as recursion won't go too deep anyway

My (completely made up) statistics suggest the exact opposite. ;-)

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Dec 10, 2016

@forki, I came to this idea from Brian on SO suggesting, esp. too beginners or moderately advanced programmers to stay away from even trying to use tail recursion: http://stackoverflow.com/a/1797302/111575.

From my own experience: looking at the my largest F# project I see some 50 or so let rec statements. Only a handful of which would benefit from this (hence my 9 out of 10, but I'm biased to myself :D). That is not to say, I also have a few while-statements which I didn't dare to make recursive for fear of getting it wrong... (shame on me, I know, kudos to this fix, it can be a life saver!)

Thinking of which: I also encountered a few mutual recursive members. Would these be detected as tailcall with this fix?

@forki
Copy link
Contributor

forki commented Dec 10, 2016 via email

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Dec 10, 2016

My train of thought was: if the tendency is to suggest beginning users to stay away from recursion, and if they don't, then stay away from tailcalls (makes sense, they can be hard, even for experienced programmers), then I think we ought to make sure this warning either doesn't throw them off, helps them, or is only available as an extra option for advanced users.

@forki
Copy link
Contributor

forki commented Dec 10, 2016

stay away from tail-calls is a bad advice. really

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Dec 10, 2016

stay away from tail-calls is a bad advice. really

I'm just trying to follow what I believe is consensus here: keep it simple for beginners, but give power tools for advanced users. Saying "stay away" is perhaps too strong, but my point is simply: don't overwhelm beginning users with trying to explain what TCO is by means of warnings. We are trying to get more users, not less.

Regardless of your opinion on my ideas of tutoriability of warnings, this warning is a very valuable addition, just don't get my comments the wrong way, I am not arguing against implementing it, or against TCO in general (apologies if it looked that way).

@theimowski
Copy link

We already have the rec keyword for marking recursive functions, so if we provide yet another construct for marking tail recursion (be it an attribute, or special keyword), it might turn out tedious to use and confusing for new-comers. That said, I'm for making tail recursion warnings turned on by default.

@TeaDrivenDev
Copy link

I'm for always checking as well, without any special keywords or decorations. Recursion is so important in functional programming that one should be able to consider it "safe" by default, so the compiler telling you when it isn't is a good thing.
As for the beginners, I think that would actually help them too. Myself, I still don't always write proper tail calls, because it often doesn't matter, so when I have to, I sometimes don't really know how to get there. If I always had to do them properly to satisfy the compiler, I'd have much more practice with that.

@OnurGumus
Copy link
Contributor

OnurGumus commented Dec 12, 2016

I would vote for a tailrec keyword. Always checking would yield unnecessary warnings.

@vasily-kirichenko
Copy link
Contributor

I'm for tailrec keyword, too. It looks quite natural as a specific rec, it's far tidier that [<TailRecursive>].

@cartermp
Copy link
Contributor

Is the suggestion here to have tailrec be a replacement for rec? e.g.

let tailrec foo() = ...

@forki
Copy link
Contributor

forki commented Dec 12, 2016 via email

@AviAvni
Copy link
Contributor Author

AviAvni commented Jan 2, 2017

@forki I can bring back the check in the code gen

@forki
Copy link
Contributor

forki commented Jan 2, 2017

I think that would be good, not sure how others feel about it.

@@ -1332,3 +1332,4 @@ tcTupleStructMismatch,"One tuple type is a struct tuple, the other is a referenc
3211,DefaultParameterValueNotAppropriateForArgument,"The default value does not have the same type as the argument. The DefaultParameterValue attribute and any Optional attribute will be ignored. Note: 'null' needs to be annotated with the correct type, e.g. 'DefaultParameterValue(null:obj)'."
tcGlobalsSystemTypeNotFound,"The system type '%s' was required but no referenced system DLL contained this type"
3213,typrelMemberHasMultiplePossibleDispatchSlots,"The member '%s' matches multiple overloads of the same method.\nPlease restrict it to one of the following:%s."
3212,chkNotTailRecursive,"The member or function '%s' has the 'TailCallAttribute' attribute, but is not being used in a tail recursive way."
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "was marked with"? Do we already have similar texts?

@dsyme
Copy link
Contributor

dsyme commented Jan 2, 2017

  1. Try to implement the tailrec intrinsic in this PR?

Ideally yes. This would be an F# 4.2 feature, and I think we would progress by first having one or two people try out the feature, e.g. try to use apply it to the F# codebase. Having both the intrinsic and the attribute implemented would help.

Note

  1. The presence of uses of the intrinsic in the TAST may easily mess up optimizations and/or codegen. So removing it during the first optimization phase makes sense. However this means you can't perform the check at code-gen time.

  2. You will also need to decide if it appears in quotations - it shouldn't I think - so should again be removed during quotation processing.

  3. Syntactically the use of an intrinsic has pros and cons. For example, peope may expect you can pipeline it, e.g. let rec f x = ... tailrec (f x) becomes let rec f x = ...f x |> tailrec. That won't work of course. I can also see people definitely asking for a keyword here, so they don't need parentheses let rec f x = ... tailrec f x

  4. The current intended rule is "TailRecAttribute implies the function is only used in a tail recursive way within its recursive scope" . Just to note that this is in tension with the various ways to increase the size of recursive scopes, e.g. namespace rec or module rec or even just a set of object-oriented type definitions. Increasing the size of a recursive scope will mean spurious errors/warnings are given. In this case, people will gravitatte to using tailrec

Checking in codegen does make sense, to make sure the optimizer hasn't screwed things around.

@forki
Copy link
Contributor

forki commented Jan 2, 2017

So intrinsics feature should be a separate PR?

@jack-pappas
Copy link
Contributor

I've been thinking -- introducing a tailrec keyword or [<TailRecursive>] attribute doesn't seem strictly necessary. Unless there's a something I'm missing, it'd only serve to augment the analysis; for example, when the keyword/attribute is applied, the analysis pass might emit an error instead of a warning.

Given this is a rather complex piece of analysis with many edge cases to consider, what if we went the route of implementing it within something like VFPT for now? There, it could be enabled optionally while we work on getting the analysis correct; once it's ready, we'd roll the analysis code into the compiler itself and implement the keyword/attribute if it still seems useful. If VFPT isn't a good fit, what about implementing the analysis as a standalone tool (again, for now) on top of FCS -- we could provide it as a NuGet package which would install the tool into a project (so no manually configuration would be needed, and it'd still be easy to iterate quickly).

@KevinRansom
Copy link
Member

KevinRansom commented Jan 17, 2017 via email

@forki
Copy link
Contributor

forki commented Jan 17, 2017

Analyzer are roslyn based. This feature is something that would useful in the whole ecosystem. We should really think about that and make the compiler smart instead of relying on analyzers. I'm not saying that we have to introduce a keyword or attribute - just that we should use analyzers only as last resort.

@KevinRansom
Copy link
Member

KevinRansom commented Jan 17, 2017 via email

@forki
Copy link
Contributor

forki commented Jan 17, 2017

I can already run as many analyzers as I want during my FAKE build. That's not the point.

@forki
Copy link
Contributor

forki commented Jan 17, 2017

Btw it's nice that people say it's a jit bug if you have to emit tail calls. Are these bugs fixed?

@forki
Copy link
Contributor

forki commented Jan 17, 2017

Even if the jitter could detect tail calls (without explicit tail call), then we still have to write it tail recursive. And the whole idea of this feature is to detect this somehow in the compiler.

@KevinRansom
Copy link
Member

@forki I believe they would consider fixing the Jit to improve code generation where they believe tail recursion should occur and it isn't. Provide a repro of where the jit does the wrong thing and they will improve it.

@dsyme
Copy link
Contributor

dsyme commented Jan 18, 2017

@jack-pappas

Unless there's a something I'm missing, it'd only serve to augment the analysis; for example, when the keyword/attribute is applied, the analysis pass might emit an error instead of a warning.

There seems to be a belief going around that F# function calls are normally tail calls and we can just warn if they aren't. This isn't the case - a huge number of F# calls (the majority of syntactic call sites) are not tail recursive - even within the recursive scope of the function or member - even more so when larger recursive scopes are being used in object-oriented programming or namespace rec.

So some kind of attribute is needed to indicate that tail recursion is intended. Otherwise we end up flagging almost everything.

@dsyme
Copy link
Contributor

dsyme commented Jan 18, 2017

Everyone - his is in the context of the approved language RFC FS-1011. We should probably put design feedback there.

@KevinRansom I'm not opposed to any particular analyzer of course, likewise I wouldn't be opposed to an FSharpLint rule. (FSharpLint rules are active in Ionide by defaut).

Writing a code fix to make something tail recursive would be hard, as there are quite a few ways to do it (see Expert F# 4.0 chapter ~15)

@forki
Copy link
Contributor

forki commented Jan 18, 2017 via email

@jack-pappas
Copy link
Contributor

@dsyme Agreed, we should move discussion about the design for this feature over to fsharp/fslang-design#82.

@jack-pappas
Copy link
Contributor

@dsyme I also realized after your comment today that we may be talking about two different things -- tail calls in general (which don't necessarily imply any recursion) and functions designed to be (mutually-)recursive (where any call sites to functions in that set of mutually-recursive functions should be tail-calls to avoid overflowing the stack). I think there's an easy way to get some low-hanging fruit on the mutually-recursive functions, and I've outlined the analysis algorithm in fsharp/fslang-design#82.

@KevinRansom
Copy link
Member

@AviAvni @dsyme

Thanks for this PR and lively discussion but it doesn't seem that we are anywhere close to having the necessary investigations and designs complete yet for a tail recursion feature.

I'm going to close this PR for now, the discussion needs to occur on user voice. When consensus or an approach has been determined, then please feel free to resubmit this or an alternate PR.

Thank you,

Kevin

@KevinRansom KevinRansom closed this Feb 3, 2017
@forki
Copy link
Contributor

forki commented Feb 3, 2017

@KevinRansom please reopen. This is one of the most interesting pull requests in recent history

@KevinRansom
Copy link
Member

The language design is
https://github.com/fsharp/fslang-design/blob/master/RFCs/FS-1011-warn-on-recursive-without-tail-call.md

This interesting thread is not being captured at the right spot.

I will add a link from :
https://github.com/fsharp/fslang-design/blob/master/RFCs/FS-1011-warn-on-recursive-without-tail-call.md

back to this thread.

Please consider continuing the discussion at the RFC.

@dsyme
Copy link
Contributor

dsyme commented Feb 4, 2017

@KevinRansom I think we could leave this open - any efforts to make progress on this particular issue are really valuable, even half-way. It's a really tricky feature to get right, and the thing we need most is a series of prototypes for people to use. A WIP PR is one way to get that (and get eyes on it)?

@realvictorprm
Copy link
Contributor

Can the FSSF help with triggering this discussion again @dsyme?

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.