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.

Sign up for GitHub

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

Jump to bottom

Crash with invalid named mixin application #48919

Open
scheglov opened this issue Apr 28, 2022 · 14 comments
Open

Crash with invalid named mixin application #48919

scheglov opened this issue Apr 28, 2022 · 14 comments
Assignees
@scheglov @chloestefantsova
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@scheglov
Copy link
Contributor 

mixin M {}
class A = void Function() with M;

main() {}

I see from dart run:

Crash when compiling file:///Users/scheglov/tmp/333/test.dart,
at character offset null:
type 'Null' is not a subtype of type 'String'
#0      extractName (package:front_end/src/fasta/source/source_library_builder.dart:5243:51)
#1      SourceLibraryBuilder.applyMixins (package:front_end/src/fasta/source/source_library_builder.dart:2147:28)
#2      SourceLibraryBuilder.addNamedMixinApplication (package:front_end/src/fasta/source/source_library_builder.dart:2352:17)
#3      OutlineBuilder.endNamedMixinApplication (package:front_end/src/fasta/source/outline_builder.dart:2105:22)
#4      Parser.parseNamedMixinApplication (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2223:14)
#5      Parser.parseClassOrNamedMixinApplication (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2202:14)
#6      Parser.parseTopLevelKeywordDeclaration (package:_fe_analyzer_shared/src/parser/parser_impl.dart:564:14)
#7      Parser.parseTopLevelDeclarationImpl (package:_fe_analyzer_shared/src/parser/parser_impl.dart:497:14)
#8      Parser.parseUnit (package:_fe_analyzer_shared/src/parser/parser_impl.dart:377:15)
#9      SourceLoader.buildOutline (package:front_end/src/fasta/source/source_loader.dart:1114:37)
<asynchronous suspension>
#10     SourceLoader.buildOutlines (package:front_end/src/fasta/source/source_loader.dart:1004:7)
<asynchronous suspension>
#11     KernelTarget.computeNeededPrecompilations.<anonymous closure> (package:front_end/src/fasta/kernel/kernel_target.dart:364:7)
<asynchronous suspension>
#12     withCrashReporting (package:front_end/src/fasta/crash.dart:122:12)
<asynchronous suspension>
#13     generateKernelInternal.<anonymous closure> (package:front_end/src/kernel_generator_impl.dart:99:11)
<asynchronous suspension>
#14     withCrashReporting (package:front_end/src/fasta/crash.dart:122:12)
<asynchronous suspension>
#15     generateKernel.<anonymous closure> (package:front_end/src/kernel_generator_impl.dart:47:12)
<asynchronous suspension>
#16     generateKernel (package:front_end/src/kernel_generator_impl.dart:46:10)
<asynchronous suspension>
#17     kernelForModule (package:front_end/src/api_prototype/kernel_generator.dart:100:11)
<asynchronous suspension>
#18     SingleShotCompilerWrapper.compileInternal (org-dartlang-kernel-service:///pkg/vm/bin/kernel_service.dart:400:11)
<asynchronous suspension>
#19     Compiler.compile.<anonymous closure> (org-dartlang-kernel-service:///pkg/vm/bin/kernel_service.dart:216:45)
<asynchronous suspension>
#20     _processLoadRequest (org-dartlang-kernel-service:///pkg/vm/bin/kernel_service.dart:894:37)
<asynchronous suspension>


#0      extractName (package:front_end/src/fasta/source/source_library_builder.dart:5243:51)
#1      SourceLibraryBuilder.applyMixins (package:front_end/src/fasta/source/source_library_builder.dart:2147:28)
#2      SourceLibraryBuilder.addNamedMixinApplication (package:front_end/src/fasta/source/source_library_builder.dart:2352:17)
#3      OutlineBuilder.endNamedMixinApplication (package:front_end/src/fasta/source/outline_builder.dart:2105:22)
#4      Parser.parseNamedMixinApplication (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2223:14)
#5      Parser.parseClassOrNamedMixinApplication (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2202:14)
#6      Parser.parseTopLevelKeywordDeclaration (package:_fe_analyzer_shared/src/parser/parser_impl.dart:564:14)
#7      Parser.parseTopLevelDeclarationImpl (package:_fe_analyzer_shared/src/parser/parser_impl.dart:497:14)
#8      Parser.parseUnit (package:_fe_analyzer_shared/src/parser/parser_impl.dart:377:15)
#9      SourceLoader.buildOutline (package:front_end/src/fasta/source/source_loader.dart:1114:37)
<asynchronous suspension>
#10     SourceLoader.buildOutlines (package:front_end/src/fasta/source/source_loader.dart:1004:7)
<asynchronous suspension>
#11     KernelTarget.computeNeededPrecompilations.<anonymous closure> (package:front_end/src/fasta/kernel/kernel_target.dart:364:7)
<asynchronous suspension>
#12     withCrashReporting (package:front_end/src/fasta/crash.dart:122:12)
<asynchronous suspension>
#13     generateKernelInternal.<anonymous closure> (package:front_end/src/kernel_generator_impl.dart:99:11)
<asynchronous suspension>
#14     withCrashReporting (package:front_end/src/fasta/crash.dart:122:12)
<asynchronous suspension>
#15     generateKernel.<anonymous closure> (package:front_end/src/kernel_generator_impl.dart:47:12)
<asynchronous suspension>
#16     generateKernel (package:front_end/src/kernel_generator_impl.dart:46:10)
<asynchronous suspension>
#17     kernelForModule (package:front_end/src/api_prototype/kernel_generator.dart:100:11)
<asynchronous suspension>
#18     SingleShotCompilerWrapper.compileInternal (org-dartlang-kernel-service:///pkg/vm/bin/kernel_service.dart:400:11)
<asynchronous suspension>
#19     Compiler.compile.<anonymous closure> (org-dartlang-kernel-service:///pkg/vm/bin/kernel_service.dart:216:45)
<asynchronous suspension>
#20     _processLoadRequest (org-dartlang-kernel-service:///pkg/vm/bin/kernel_service.dart:894:37)
<asynchronous suspension>
@scheglov scheglov added the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label Apr 28, 2022
@scheglov
Copy link
Contributor Author

Similar code causes a crash in the analyzer AstBuilder (lines might be slightly different):

  type 'GenericFunctionTypeImpl' is not a subtype of type 'NamedType' in type cast
  #0      AstBuilder.endNamedMixinApplication (package:analyzer/src/fasta/ast_builder.dart:2146:26)
  #1      Parser.parseNamedMixinApplication (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2223:14)
  #2      Parser.parseClassOrNamedMixinApplication (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2202:14)
  #3      Parser.parseTopLevelKeywordDeclaration (package:_fe_analyzer_shared/src/parser/parser_impl.dart:564:14)
  #4      Parser.parseTopLevelDeclarationImpl (package:_fe_analyzer_shared/src/parser/parser_impl.dart:497:14)
  #5      Parser.parseUnit (package:_fe_analyzer_shared/src/parser/parser_impl.dart:377:15)
  #6      Parser.parseCompilationUnit2 (package:analyzer/src/generated/parser.dart:108:32)
  #7      Parser.parseCompilationUnit (package:analyzer/src/generated/parser.dart:104:12)
  #8      FileState._parse (package:analyzer/src/dart/analysis/file_state.dart:550:23)

Which raises a question - does the parser know whether it is a function type or a named type? If it does, it would be nice to report an error and recover.

@scheglov
Copy link
Contributor Author

scheglov commented Apr 28, 2022 •
edited

And similar issue with extends void Function(), and least in the analyzer.

  type 'GenericFunctionTypeImpl' is not a subtype of type 'NamedType?' in type cast
  #0      AstBuilder.handleClassExtends (package:analyzer/src/fasta/ast_builder.dart:2725:27)
  #1      Parser.parseClassExtendsSeenExtendsClause (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2391:14)
  #2      Parser.parseClassExtendsOpt (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2362:15)
  #3      Parser.parseClassHeaderOpt (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2255:13)
  #4      Parser.parseClass (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2242:13)
  #5      Parser.parseClassOrNamedMixinApplication (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2206:14)
  #6      Parser.parseTopLevelKeywordDeclaration (package:_fe_analyzer_shared/src/parser/parser_impl.dart:564:14)
  #7      Parser.parseTopLevelDeclarationImpl (package:_fe_analyzer_shared/src/parser/parser_impl.dart:497:14)
  #8      Parser.parseUnit (package:_fe_analyzer_shared/src/parser/parser_impl.dart:377:15)
  #9      Parser.parseCompilationUnit2 (package:analyzer/src/generated/parser.dart:108:32)
  #10     Parser.parseCompilationUnit (package:analyzer/src/generated/parser.dart:104:12)
  #11     FileState._parse (package:analyzer/src/dart/analysis/file_state.dart:550:23)

@scheglov scheglov added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Apr 28, 2022
@scheglov
Copy link
Contributor Author

...and for with clause.

  type 'GenericFunctionTypeImpl' is not a subtype of type 'NamedType' in type cast
  #0      StackImpl.popList (package:_fe_analyzer_shared/src/parser/stack_listener.dart:597:25)
  #1      AstBuilder.popTypedList (package:analyzer/src/fasta/ast_builder.dart:4140:11)
  #2      AstBuilder.endTypeList (package:analyzer/src/fasta/ast_builder.dart:2505:10)
  #3      Parser.parseTypeList (package:_fe_analyzer_shared/src/parser/parser_impl.dart:975:14)
  #4      Parser.parseClassWithClauseOpt (package:_fe_analyzer_shared/src/parser/parser_impl.dart:1270:15)
  #5      Parser.parseClassHeaderOpt (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2256:13)
  #6      Parser.parseClass (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2242:13)
  #7      Parser.parseClassOrNamedMixinApplication (package:_fe_analyzer_shared/src/parser/parser_impl.dart:2206:14)
  #8      Parser.parseTopLevelKeywordDeclaration (package:_fe_analyzer_shared/src/parser/parser_impl.dart:564:14)
  #9      Parser.parseTopLevelDeclarationImpl (package:_fe_analyzer_shared/src/parser/parser_impl.dart:497:14)
  #10     Parser.parseUnit (package:_fe_analyzer_shared/src/parser/parser_impl.dart:377:15)
  #11     Parser.parseCompilationUnit2 (package:analyzer/src/generated/parser.dart:108:32)
  #12     Parser.parseCompilationUnit (package:analyzer/src/generated/parser.dart:104:12)
  #13     FileState._parse (package:analyzer/src/dart/analysis/file_state.dart:550:23)

@johnniwinther johnniwinther added the P2 A bug or feature request we're likely to work on label Apr 29, 2022
@jensjoha
Copy link
Contributor

jensjoha commented May 9, 2022

@johnniwinther why was I assigned to this?

I initially thought it was some sort of parsing error, but the parser seems to conform to the spec to me, e.g.

<classDeclaration> ::=
  \ABSTRACT? \CLASS{} <identifier> <typeParameters>?
  \gnewline{} <superclass>? <interfaces>?
  \gnewline{} `{' (<metadata> <classMemberDeclaration>)* `}'
  \alt \ABSTRACT? \CLASS{} <mixinApplicationClass>

<mixinApplicationClass> ::= \gnewline{}
  <identifier> <typeParameters>? `=' <mixinApplication> `;'

<mixinApplication> ::= <typeNotVoid> <mixins> <interfaces>?

<interfaces> ::= \IMPLEMENTS{} <typeNotVoidList>

<superclass> ::= \EXTENDS{} <typeNotVoid> <mixins>?
    \alt <mixins>

<mixins> ::= \WITH{} <typeNotVoidList>

<typeNotVoidList> ::= <typeNotVoid> (`,' <typeNotVoid>)*

<typeNotVoid> ::= <functionType> `?'?
  \alt <typeNotVoidNotFunction>

<functionType> ::= <functionTypeTails>
  \alt <typeNotFunction> <functionTypeTails>

<functionTypeTails> ::= <functionTypeTail> `?'? <functionTypeTails>
  \alt <functionTypeTail>

<functionTypeTail> ::= \FUNCTION{} <typeParameters>? <parameterTypeList>

<typeNotFunction> ::= \VOID{}
  \alt <typeNotVoidNotFunction>

As I read it all of these should be OK:

mixin M {}
class X {}

class A = void Function() with M;
class B = void Function() with void Function();
class C = void Function() with void Function() implements void Function();
class D extends X with void Function() {}
class E extends void Function() {}

main() {}

@johnniwinther
Copy link
Member

This sounds like this is an issue in both the CFE and analyzer. Reassigning.

@scheglov
Copy link
Contributor Author

scheglov commented May 9, 2022

@eernstg Is this correct? Do we allow function types as superclasses? Just syntactically or also semantically?

It looks that 10.9 forbids using type aliases that are not classes, but nothing about function types written directly.
image

@eernstg
Copy link
Member

eernstg commented May 9, 2022 •
edited

It is definitely intended that a function type as a superclass should be a compile-time error. The change which was made in order to ensure that it has no effect if a class implements Function (and otherwise uses Function as a superinterface) was intended to ensure that no expression whose static type is Function or a function type would ever yield any other kind of object than a function object (so an object can't pretend to be a function object). This means that it is possible to use a faster approach to invoke function objects than the approach where it is done as a virtual method invocation of call.

The PR where null safety is integrated into the language specification is ongoing work (a snippet of it is available at dart-lang/language#2023), but it contains a short section defining 'class building types'. That section corrects a couple of mistakes and updates the definitions to take null safety into account, because it specifies that void is not a class building type, T? is not a class building type for any T, and no function type is a class building type.

The concept of being a class building type is then used in the sections about extends, with and implements, including the location you mention.

This means that function types aren't mentioned today, but that's clearly a mistake, and it is being corrected when the null safety updates are landed (reviews are ongoing, we will get there ;-).

copybara-service bot pushed a commit that referenced this issue May 11, 2022
@chloestefantsova
[cfe] Require a non-null name for mixin supertypes
aa9b83e 
The function types are given an empty name in that case. Those types
are further discarded and can have any intermediary name.

Part of #48919

Change-Id: I57d223ee7914d0227baa3a4ef5733bb8055fa2c3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/244240
Commit-Queue: Chloe Stefantsova <cstefantsova@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
@chloestefantsova
Copy link
Contributor

The fix for the CFE has landed: https://dart-review.googlesource.com/c/sdk/+/244240.

@scheglov
Copy link
Contributor Author

If this is a compile-time error to use a function type as a superclass, then should the parser report this error instead? When we see that this is an explicit function type, not a type alias, of course.

@bwilkerson
Copy link
Member

The grammar currently includes the following:

<superclass> ::= extends <typeNotVoid> <mixins>?
    | <mixins>
<typeNotVoid> ::= <functionType> ‘?’?
    | <typeNotVoidNotFunction>

While it would make sense to me for the grammar to exclude function types in the extends clause (@eernstg), it currently doesn't. Because the grammar allows it, IMO it shouldn't be a parse error.

@scheglov
Copy link
Contributor Author

My understanding was that it is an oversight that the grammar does not exclude function types. And because we know this, it does not seem right to hold to the letter of the current grammar and insist that CFE and analyzer should do their own error reporting for invalid syntax here.

I did this initially in the analyzer, but thought that it might be beneficial to check syntax in one place - in the parser.

@eernstg
Copy link
Member

eernstg commented May 12, 2022

I think it would be complicating matters (slightly) for no good reason if we were to make it a syntax error: As @scheglov mentioned, it would still be possible to use a function type as a superinterface via a type alias, so we would still have to check syntactic forms that aren't a <functionType>. A syntax error would just catch that same error a bit earlier.

So unless there are grammatical reasons for doing it (parser speed? disambiguation?), I think we shouldn't build double-walls against dangers when one wall will do just fine. ;-)

@renatoathaydes
Copy link

I saw this issue on emacs so I had reported it on the lsp project: emacs-lsp/lsp-dart#191

Just declaring a mixin caused the LSP to keep dying all the time.

@renatoathaydes
Copy link

Maybe this is related: The Dart compiler doesn't seem to auto-cast this:

mixin Foo {
  void bar();
}

class FooFunction with Foo {
  void call() {}
  void bar() {}
}

main() {
  Function() fun = FooFunction();
  
  if (fun is Foo) {
    fun.bar();
  }
}

Fails with:

Error compiling to JavaScript:
Info: Compiling with sound null safety
lib/main.dart:14:9:
Error: The method 'bar' isn't defined for the class 'dynamic Function()'.
    fun.bar();
        ^^^
Error: Compilation failed.

An explicit cast is needed:

  if (fun is Foo) {
    (fun as Foo).bar();
  }

So there's something funky about functions and mixins?!

@srawlins srawlins added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@scheglov scheglov

@chloestefantsova chloestefantsova

Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@srawlins @scheglov @chloestefantsova @renatoathaydes @bwilkerson @eernstg @johnniwinther @jensjoha