Skip to content

Conversation

@eernstg
Copy link
Member

@eernstg eernstg commented Oct 22, 2025

See #4485 for background information.

This PR changes the feature specification of declaring constructors such that its grammar rules support the new style of constructor declarations (like new(); and const factory name() = D;).

It handles the ambiguity among factory(); being a method or a factory constructor declaration whose name is the name of the enclosing class/mixin-class/enum/extension-type declaration by making factory a reserved word.

@eernstg eernstg requested a review from munificent October 22, 2025 10:36
@eernstg
Copy link
Member Author

eernstg commented Oct 22, 2025

Testing the libraries in tests/language, the fact that factory is now a reserved word gives rise to 23 new parsing failures (we already have some failures because of parameters declared as var p). Apparently, there are no methods named factory here.

A couple of errors arise because the test specifically tests the treatment of built-in identifiers and reserved words, so that's to be expected.

The remaining errors are all concerned with the use of factory as the second part of a constructor name (as in class C { factory C.factory() => ...; }). This could occur in real code, but it seems likely that it is a more natural choice in test code, and not so common in production.

We could try out the radical approach (making factory a reserved word) in internal code and see if it causes non-trivial breakage (a quick search yields about 12 constructors whose name is of the form C.factory, apparently almost all of them in testing code). Alternatively, we could allow factory everywhere except as a type name and as the name of a method (which yields minimal breakage).

@eernstg eernstg force-pushed the spec_new_constructors_oct25 branch from c1abdba to 19471aa Compare October 23, 2025 13:26
Copy link
Member

@munificent munificent left a comment

Choose a reason for hiding this comment

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

This looks great!

<classDeclaration> ::= // First alternative modified.
(<classModifiers> | <mixinClassModifiers>)
'class' <classNamePart> <superclass>? <interfaces>? <classBody>
'class' <classNameMaybePrimary> <superclass>? <interfaces>? <classBody>
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion/nit: "maybe" implies "optional" to me but not a choice from two alternatives. Perhaps a better name is "classNameOrPrimary"?

<typeIdentifier> <typeParameters>?
('.' <identifierOrNew>)? <declaringParameterList>
<primaryConstructor> ::= // New rule.
'const'? <typeWithParameters> ('.' <identifierOrNew>)?
Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, I never thought there was value in supporting Foo.new in constructor declarations, and we don't necessarily have to support it for primary constructors. We could just do ('.' <identifier>) here.

But if you think it's worth it for consistency, I could go either way. :)

*This allows the new constructor declaration syntax such as `factory();` to
be unambiguous. It is also a breaking change because `factory` can no
longer be the name of a declaration. However, this occurs only infrequently
today.*
Copy link
Member

Choose a reason for hiding this comment

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

We should be explicit here that this is a language versioned change. We'd say that factory is a built-in identifier in pre-feature libraries and a reserved word in later versions.

<constantConstructorSignature> ::= // Modified rule.
: 'const' <constructorName> <formalParameterList> // Old form.
| 'const' <constructorHead> <formalParameterList> // New form.
Copy link
Member

Choose a reason for hiding this comment

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

All the "old form" / "new form" comments are very helpful here.

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.

3 participants