Parens: handle special parsing of new T… & inherit T…

[brianrourkeboll]

Followup to #16079.

• Disallow removing parentheses from around certain constructor argument expressions when the constructor is being invoked after the new or inherit keywords.

Constructor applications are handled differently in the parser after the new and inherit keywords than they are elsewhere: only a subset of expressions are allowed as an argument (as specified in atomicExprAfterType in pars.fsy). This means that any expression outside of that blessed subset must be parenthesized, even if the exact same ctor application would not require parentheses without new or inherit.

E.g.,

let x = 3
let _ = ResizeArray<int>(x)     // Fine.
let _ = ResizeArray<int> x      // Fine.
let _ = ResizeArray<int> 3      // Fine.
let _ = new ResizeArray<int>(x) // Fine.
let _ = new ResizeArray<int> x  // Not fine.
let _ = new ResizeArray<int> 3  // Fine.

let _ = ResizeArray<int>([|3|])     // Fine.
let _ = ResizeArray<int> [|3|]      // Fine.
let _ = ResizeArray<int>([3])       // Fine.
let _ = ResizeArray<int> [3]        // Fine.
let _ = new ResizeArray<int>([|3|]) // Fine.
let _ = new ResizeArray<int> [|3|]  // Fine.
let _ = new ResizeArray<int>([3])   // Fine.
let _ = new ResizeArray<int> [3]    // Not fine.

(* Likewise in object expressions (where `new` is required), etc. *)

type E (s : string) = inherit exn(s)     // Fine.
type E (s : string) = inherit exn $"{s}" // Fine.
type E (s : string) = inherit exn s      // Not fine.

This is due to the following in pars.fsy:

fsharp/src/Compiler/pars.fsy

Line 4841 in 24ef671

| NEW atomTypeNonAtomicDeprecated opt_HIGH_PRECEDENCE_APP atomicExprAfterType

fsharp/src/Compiler/pars.fsy

Line 5336 in 24ef671

| INHERIT atomTypeNonAtomicDeprecated opt_HIGH_PRECEDENCE_APP opt_atomicExprAfterType recdExprBindings opt_seps_recd

fsharp/src/Compiler/pars.fsy

Lines 5042 to 5076 in 24ef671

	/* the start of atomicExprAfterType must not overlap with the valid postfix tokens of the type syntax, e.g. new List<T>(...) */
	atomicExprAfterType:
	| constant
	{ SynExpr.Const(fst $1, snd $1) }
	| parenExpr
	{ $1 }
	| braceExpr
	{ $1 }
	| braceBarExpr
	{ $1 }
	| interpolatedString
	{ let parts, synStringKind = $1
	SynExpr.InterpolatedString(parts, synStringKind, rhs parseState 1) }
	| NULL
	{ SynExpr.Null(lhs parseState) }
	| FALSE
	{ SynExpr.Const(SynConst.Bool false, lhs parseState) }
	| TRUE
	{ SynExpr.Const(SynConst.Bool true, lhs parseState) }
	| quoteExpr
	{ $1 }
	| arrayExpr
	{ $1 }
	| beginEndExpr
	{ $1 }

These are required to support the use of postfix generic notation in those contexts (as noted in #16239 (comment)) and possibly other things. Even though the use of postfix notation in such contexts seems likely to be quite rare on the whole, removing support for it would be a breaking change.

Edit: click to see the question that was answered in the comments

Question for those in the know

(I can open a new issue for this if it makes sense.)

I've run into this inconsistency repeatedly over the years and it has always bothered me. Is the atomicExprAfterType restriction here actually still needed? Would that suddenly cause more problems like those addressed by #15923? Or could we just replace it with argExpr in the parser for new and inherit and get rid of the inconsistency? Making that change locally seems to work and doesn't seem to cause any immediate problems in this codebase...

...Ideally new ctor expr and ctor expr would be completely interchangeable (except, probably, in object expressions), but there is one additional inconsistency that would require more changes to the compiler to resolve. Explicit type arguments are required when the application is preceded by new but can be inferred when new is not present: ResizeArray 3 is allowed, but new ResizeArray (3) is not (regardless of parentheses around the arg).

[T-Gro]

I have a fear that relaxing this altogether would lead to a lot more parser issues, especially w.r.t to parser precedence rules and type arguments.

let x = new (int A)(new (int B)(new (int C))))

When you tried to change the parser to accommodate this, all test suites including FsharpQA ran without issue?

[brianrourkeboll]

@T-Gro ah, yeah, of course: postfix notation 🙃

fsharp/tests/fsharpqa/Source/Conformance/Expressions/ApplicationExpressions/BasicApplication/E_PostfixType01.fs

Lines 6 to 7 in 24ef671

	type T<'t> = System.Collections.Generic.List<'t>
	let o = new int T ()

fsharp/tests/FSharp.Compiler.ComponentTests/Conformance/Expressions/ApplicationExpressions/BasicApplication/E_PostfixType03.fs

Lines 6 to 10 in 24ef671

	type T<'t> = System.Collections.Generic.List<'t>
	type C<'t> () = class
	inherit int T ()
	end

That means that this PR is needed, then.

[psfinaki]

Thanks, great!