Skip to content

Function is a built-in identifier, but no error is emitted when it is used to name a type #45703

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

Closed
eernstg opened this issue Apr 14, 2021 · 14 comments
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). Epic

Comments

@eernstg
Copy link
Member

eernstg commented Apr 14, 2021

Consider the following programs:

class Function {}
class C<Function> {}
void main() {}
typedef Function = int;
typedef F<Function> = int;
void main() {}
extension Function on List {}
extension E<Function> on List<Function> {}
void main() {}
mixin Function {}
mixin M<Function> implements List<Function> {}
void main() {}

The word Function is a built-in identifier, but it is not consistently reported as an error when this word is used as the name of a type (that is, a class, type parameter, type alias, etc.).

No errors or other diagnostics are emitted except for the following:

  • The analyzer hints that a class named Function is deprecated.
  • The analyzer and the CFE emit an error "'Function' can't be used as an identifier because it's a keyword" on typedef Function = int;.

All declaring occurrences of Function should be marked as a compile-time error.

@jensjoha
Copy link
Contributor

The link tries to point to line 16025 in a file with 14154 lines in it... Could you provide a better link?

@lrhn
Copy link
Member

lrhn commented Apr 19, 2021

The link is correct, but Github sometimes doesn't show the whole file (it's 21899 lines). Try reloading.
(It's also not a particularly interesting link, it just points to the line saying \alt \FUNCTION{} in the list of built-in identifiers.)

@jensjoha
Copy link
Contributor

Hah, fantastic! Thanks!

@scheglov
Copy link
Contributor

scheglov commented May 13, 2021

There is class Function in lib/core/function.dart, and another in dart-sdk/lib/_internal/vm/lib/function_patch.dart.
Do we make an exception for SDK?

@lrhn
Copy link
Member

lrhn commented May 14, 2021

As I remember it (big caveat), we did not choose to make Function a built-in identifier. Instead we treated Function followed by ( or < as a context-sensitive keyword instead of an identifier.

One of the reasons was that Function is a type name already, and you need to be able to use it as one, which you can't with built-in identifiers. Did we change the approach to "built-in identifier but can be used as type" instead?

@bwilkerson
Copy link
Member

Section 17.37 Identifier Reference, as of May 6, lists Function as a built-in identifier. I don't know whether that's as intended.

@eernstg

@lrhn
Copy link
Member

lrhn commented May 14, 2021

Nope, my bad. Function has apparently been a built-in identifier since Dart 2.0. Don't think I ever remembered that.
Introduced in https://dart-review.googlesource.com/c/sdk/+/41360/9/docs/language/dartLangSpec.tex

In which case the platform "class" declaration for Function must be special cased (just like FutureOr, because it's not really class, or at least it doesn't have to be).

@eernstg
Copy link
Member Author

eernstg commented May 15, 2021

Yes, Function was made a built-in identifier Feb 19 2018 in cec672c. I don't remember the exact discussions about this change. We already have another built-in identifier which typically denotes a type: dynamic. So in that sense Function is not alone. I don't see a problem in the fact that a system library contains a class Function ... — that declaration is given special treatment by the analyzer/compilers anyway.

We do get an error for a type alias, for the CFE as well as the analyzer:

// Store in `n014.dart`.
typedef Function = int;
void main() {}
/*
> dart n014.dart
n014.dart:1:9: Error: 'Function' can't be used as an identifier because it's a keyword.
Try renaming this to be an identifier that isn't a keyword.
typedef Function = int;
        ^^^^^^^^
> dartanalyzer n014.dart
Analyzing n014.dart...
  error • 'Function' can't be used as an identifier because it's a keyword. • n014.dart:1:9 • expected_identifier_but_got_keyword
1 error found.
*/

We do not get that error for a class or a mixin, but the analyzer reports that it is "deprecated" to use the name Function for a class. In other words, it's not fully implemented today.

So do we wish to make it a regular identifier in the spec, given that the error is not emitted for classes an mixins? I don't think it will create any serious parsing difficulties because Function must be handled when it occurs as a type anyway, but I doubt that it will be helpful in a software engineering sense to have a user-written class or mixin named Function.

@bwilkerson
Copy link
Member

I'll just note that there is a CL to start generating diagnostics in analyzer in these cases (https://dart-review.googlesource.com/c/sdk/+/195760).

Does the fact that we use Function for function types, such as int Function(String), impact whether it's a built-in identifier or a normal identifier?

@scheglov
Copy link
Contributor

I started this in https://dart-review.googlesource.com/c/sdk/+/200080, but it seems that there are more changes to CFE than I can do on my own.

@lrhn
Copy link
Member

lrhn commented May 15, 2021

Does the fact that we use Function for function types, such as int Function(String), impact whether it's a built-in identifier or a normal identifier?

No, when followed by < or (, the Function token is effectively a contextual keyword instead of an identifier. It's not resolved against the environment at all.

@leafpetersen
Copy link
Member

I see that we issue a hint on this now, but we also seem to issue the hint for the version that is in the SDK core libraries (which, from conversation above, is supposed to be exempt?).

@bwilkerson
Copy link
Member

Yes, we're producing a hint for class declarations (HintCode.DEPRECATED_FUNCTION_CLASS_DECLARATION), but we ought to be producing one of the CompileTimeErrorCode BUILT_IN_IDENTIFIER_AS_* codes in order to catch its use in other places. For example, I believe we should produce a diagnostic for the following, but we don't:

extension Function on int {}

@stereotype441 In case you'd be interested in fixing this, given that it spans both analyzer and CFE.

copybara-service bot pushed a commit that referenced this issue Jun 28, 2022
This CL makes `Function` a builtIn keyword instead of a `pseudo` keyword.

See also (and fixes):
#45703
#45704
#45705
#49197

This undoes https://dart-review.googlesource.com/c/sdk/+/195761

This is ~a merge of https://dart-review.googlesource.com/c/sdk/+/195906
and https://dart-review.googlesource.com/c/sdk/+/200080

Change-Id: I8bfee6976d43819fa355de99b3b2429eb67a7cdd
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/249484
Reviewed-by: Johnni Winther <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Jens Johansen <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
@jensjoha
Copy link
Contributor

This should be fixed with 25dcfca.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-meta Cross-cutting, high-level issues (for tracking many other implementation issues, ...). Epic
Projects
None yet
Development

No branches or pull requests

6 participants