Warning or error should be raised when alternative constructor calls itself

[markpattison]

The language spec (8.6.3) says that any additional constructor should call another constructor.

It's possible for an additional constructor to call itself (presumably accidentally on the part of the programmer), which results in a stack overflow.

Repro steps

type MyClass(a, b) =
    new(a) = MyClass(a)

let x = MyClass(1)

Expected behavior

Compiler should issue a warning or error when a constructor calls itself.

Actual behavior

StackOverflowException is thrown.

Known workarounds

N/A

Related information

Windows 7, F#4.0, .NET 4.5.2
Severity = low

[dsyme]

Linking master "error improvement" bug #1103

// cc @isaacabraham

[realvictorprm]

I'm trying to play around with this, maybe something can be done here?
TypeChecker.fs, Line 10958

....
and AnalyzeRecursiveStaticMemberOrValDecl (cenv, envinner: TcEnv, tpenv, declKind, newslotsOK, overridesOK, tcrefContainerInfo, vis1, id:Ident, vis2, declaredTypars, memberFlagsOpt, thisIdOpt, bindingAttribs, valSynInfo, ty, bindingRhs, mBinding, flex) =
    let vis = CombineVisibilityAttribs vis1 vis2 mBinding

    // Check if we're defining a member, in which case generate the internal unique 
    // name for the member and the information about which type it is agumenting 
      
    match tcrefContainerInfo, memberFlagsOpt with 
    | (Some(MemberOrValContainerInfo(tcref, optIntfSlotTy, baseValOpt, _safeInitInfo, declaredTyconTypars)),Some memberFlags) -> 
        assert (Option.isNone optIntfSlotTy)
      
        CheckMemberFlags cenv.g None newslotsOK overridesOK memberFlags id.idRange
        CheckForNonAbstractInterface declKind tcref memberFlags id.idRange

        if memberFlags.MemberKind = MemberKind.Constructor && tcref.Deref.IsExceptionDecl then
            error(Error(FSComp.SR.tcConstructorsDisallowedInExceptionAugmentation(),id.idRange))                  

        let isExtrinsic = (declKind = ExtrinsicExtensionBinding)
        let _,enclosingDeclaredTypars,_,objTy,thisTy = FreshenObjectArgType cenv mBinding TyparRigidity.WillBeRigid tcref isExtrinsic declaredTyconTypars
        let envinner = AddDeclaredTypars CheckForDuplicateTypars enclosingDeclaredTypars envinner
        let envinner = MakeInnerEnvForTyconRef cenv envinner tcref isExtrinsic 

        let safeThisValOpt, baseValOpt = 
            match memberFlags.MemberKind with 
            
            // Explicit struct or class constructor
            | MemberKind.Constructor  ->
...

[ScottHutchinson]

I'm hoping to add a warning. It appears that it could go right about here:
https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/TypeChecker.fs#L3945

Also there is this comment at L10539:

// If binding a ctor then set the ugly counter that permits us to write ctor expressions on the r.h.s.

I assume we want to limit that to write non-recursive ctor expressions on the r.h.s.

Now I need to figure out how to match a recursive call. I'm looking for an example somewhere else in the TypeChecker.fs file, but it's not jumping out at me.

[ScottHutchinson]

Another approach to this might be to make a constructor definition like a let binding, in which the symbol being defined is not in scope unless the rec keyword is used. The tricky part of this approach is that the symbol being defined is "new", but the recursive call is "MyClass" (or whatever the class is named). Also constructors can be overloaded, whereas let bindings cannot. That could also complicate detecting the recursive call, but that problem must be solved for any solution.

In the case of Issue #3831, the let binding itself is not even recursive, but the body calls a constructor, which then indirectly calls the let binding again, resulting in a StackOverflowException.

I might be difficult to solve both issues with one solution, so I think I will focus on solving this one first. Then the other issue might require a generalization of this solution or a different solution altogether.

[charlesroddie]

I don't see the problem here or the similar subsequent issues (#8623, #3831). Constructors are recursive in the sense that a constructor can see constructors defined both above and below. To change that would be very breaking. It then makes sense that they can see themselves. (@ScottHutchinson 's rec idea is good but would need new syntax to allow existing syntax to keep working recursively.)

The language spec (8.6.3) says that any additional constructor should call another constructor... It's possible for an additional constructor to call itself

The spec does not disallow a constructor from calling itself. It only says that it should call another constructor.

For example, this is OK:

type Foo() =
    new(x:bool) = if x then Foo(not x) else Foo()

The spec is right that the following code is bad as it is guaranteed not to terminate:

type Foo() =
    new(x:bool) = Foo(x)

It's logical that a constructor that only calls itself should compile and fail at runtime. A robust check that a constructor will eventually call another constructor is not possible (halting problem). A warning would be sensible , but that should be a larger language request as let rec g() = g(): unit currently compiles without a warning too .

[abelbraaksma]

A robust check that a constructor will eventually call another constructor is not possible (halting problem)

@charlesroddie The halting problem deals with whether an algorithm ever finishes. That's indeed not possible, but I don't think that's requested here.

In fact, all that's needed is a relatively simple recursive descent, where a hashset is threaded through with the visited constructors. We could simplify that further by only looking at tail position (it's possible to call the same constructor in another position, but that's really fringe, and besides, that doesn't remove the requirement of calling a constructor in tail position). Hierarchies of 5 or more are rare, so this should be relatively cheap.

Of course we should then raise a warning and not an error.

but that should be a larger language request as let rec g() = g(): unit currently compiles without a warning too.

I disagree. let rec is an explicit request for recursion. Showing then a warning when you're recursing is counter intuitive. And for warning on non-termination: that is the halting problem.

In contrast, a constructor typically will not recurse, and in most cases shouldn't (I can't think of a legitimate reason to have to recurse in constructors, the task is to instruct the CLR that an object needs to be created. Doing that nested more than once and if the same object, seems off, but I maybe wrong). Hence a warning makes sense, as the user probably didn't intend to do it.