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.

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

Analyzer crashing while processing macro usage #55756

Closed
helightdev opened this issue May 17, 2024 · 9 comments
Closed

Analyzer crashing while processing macro usage #55756

helightdev opened this issue May 17, 2024 · 9 comments
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@helightdev
Copy link

While playing around with macro experiment, I found following analyzer crash:

[11:55:10 AM] [Analyzer] [Error] [UnimplementedError: Null
#0      DeclarationBuilder.resolveIdentifier (package:analyzer/src/summary2/macro_declarations.dart:308:9)
#1      _Builder._buildIdentifier (package:_macros/src/executor/augmentation_library.dart:73:53)
#2      _Builder._buildCode (package:_macros/src/executor/augmentation_library.dart:133:9)
#3      _Builder._buildCode (package:_macros/src/executor/augmentation_library.dart:131:9)
#4      _Builder._buildCode (package:_macros/src/executor/augmentation_library.dart:131:…

Full Log
Full Reference Code

This analyzer only crashes once I uncomment builder.declareInType(metaCode) in content.dart.

Future introspectTest(
    ClassDeclaration clazz, MemberDeclarationBuilder builder) async {
  /// [...]
  var methods = await builder.methodsOf(clazz);
  for (var method in methods) {
    /// [...]
    var annotations = method.metadata.map(rebuildMetadata).toList();

    var metaCode = DeclarationCode.fromParts([
      "dynamic meta_",
      method.identifier.name,
      " = ",
      "[",
      ...annotations.expand((e) => [e, ","]),
      "];"
    ]);
    //builder.declareInType(metaCode);
  }
}

Code rebuildMetadata(MetadataAnnotation annotation) {
  if (annotation is IdentifierMetadataAnnotation) {
    return RawCode.fromParts([annotation.identifier]);
  } else if (annotation is ConstructorMetadataAnnotation) {
    var parts = <Object>[annotation.type.code, "("];
    for (var arg in annotation.positionalArguments) {
      parts.add(arg);
      parts.add(",");
    }
    for (var arg in annotation.namedArguments.entries) {
      parts.add(arg.key);
      parts.add(":");
      parts.add(arg.value);
      parts.add(",");
    }
    parts.add(")");
    return RawCode.fromParts(parts);
  }
  return RawCode.fromString("null");
}
@lrhn lrhn added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label May 17, 2024
@scheglov
Copy link
Contributor

I can reproduce this.
image

Code

import 'package:macro_test/macro_test.dart';

@ControllerMacro()
class MyControllerText {
  @CustomAnnotation()
  void main() {}
}

class CustomAnnotation {
  const CustomAnnotation();
}

The reason it crashes is that the macro attempts to access the identifier @CustomAnnotation during the declarations phase. At this time this identifier is not resolved yet - we resolve metadata after the declarations phase, but before the definitions phase.

The reason we cannot resolve annotations until after the declarations phase is that resolution potentially depends on constructors that might be added at any point during the declarations phase.

From the macro API implementation inside the analyzer we don't have any better solution than to throw - macro.ResolvedIdentifier resolveIdentifier() must return something, and if the identifier cannot be resolved, there is nothing to return.

@jakemac53 For any API suggestions.

@scheglov scheglov self-assigned this May 17, 2024
@scheglov scheglov added the P1 A high priority bug; for example, a single project is unusable or has many test failures label May 17, 2024
@scheglov
Copy link
Contributor

With https://dart-review.googlesource.com/c/sdk/+/366966 we will tell the name of the unresolved identifier.

Theoretically we know much more - the exact AST node (so the offset), and the content of the file. So, if this happens regularly, we can provide more details to simplify identification of the problematic identifier.

copybara-service bot pushed a commit that referenced this issue May 17, 2024
Bug: #55756
Change-Id: Ia1eab2bdf8900843151386f7dc31423e5f62805a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/366966
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
@jakemac53
Copy link
Contributor

The reason we cannot resolve annotations until after the declarations phase is that resolution potentially depends on constructors that might be added at any point during the declarations phase.

The type is definitely already known, and it seems like we should be able to then know the assumed "identifier" for any constructor (it must be a const constructor with that name on that known type). So, while you can't tie the constructor identifier to a specific AST node or element, we should know everything we need to know to output a reference to that identifier as Code, which is the important part (the import URI for it plus the scope (class name) and the name of the constructor)?

@scheglov
Copy link
Contributor

True, we should be able to resolve the type element.

@scheglov
Copy link
Contributor

BTW, here is the script that I use to see if the example runs well, and generated code.

import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/src/dart/analysis/analysis_context_collection.dart';

void main() async {
  var collection = AnalysisContextCollectionImpl(
    includedPaths: [
      '/Users/scheglov/tmp/2024-05-17/AnalyzerCrashResolveIdentifier/lib/controller.dart',
    ],
  );

  var timer = Stopwatch()..start();
  for (var analysisContext in collection.contexts) {
    print(analysisContext.contextRoot.root.path);
    var analysisSession = analysisContext.currentSession;
    for (var path in analysisContext.contextRoot.analyzedFiles()) {
      if (path.endsWith('.dart')) {
        var libResult = await analysisSession.getResolvedLibrary(path);
        if (libResult is ResolvedLibraryResult) {
          for (var unitResult in libResult.units) {
            print('    ${unitResult.path}');
            var ep = '\n        ';
            print('      errors:$ep${unitResult.errors.join(ep)}');
            print('---');
            print(unitResult.content);
            print('---');
          }
        }
      }
    }
  }
  print('[time: ${timer.elapsedMilliseconds} ms]');

  await collection.dispose();
}

@scheglov
Copy link
Contributor

copybara-service bot pushed a commit that referenced this issue May 17, 2024
…ifier.

Bug: #55756
Change-Id: Ibde1628dde8d08f95c46ee7c21332a6cf4ce6bca
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/366895
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@helightdev
Copy link
Author

helightdev commented May 17, 2024

The solution now indeed works for the const constructor annotations, though using a reference to a global const field still results in a crash.

import 'package:macro_test/macro_test.dart';

@ControllerMacro()
class MyControllerText {
  @CustomAnnotation()
  @refAnnotation
  void main() {

  }
}

class CustomAnnotation {
  const CustomAnnotation();
}

const refAnnotation = CustomAnnotation();

This code now fails with

Invalid argument(s): Unresolved identifier: ref
#0      DeclarationBuilder.resolveIdentifier (package:analyzer/src/summary2/macro_declarations.dart:308:9)
#1      _Builder._buildIdentifier (package:_macros/src/executor/augmentation_library.dart:73:53)
#2      _Builder._buildCode (package:_macros/src/executor/augmentation_library.dart:133:9)
#3      _Builder._buildCode (package:_macros/src/executor/augmentation_library.dart:131:9)
#4      _Builder.build (package:_macros/src/executor/augmentation…

As expected since 2fa0501 now outputs the reference to the offending identifier.

Could there be a possibility to also assume the identifier code of the const field similarly to how constructor identifiers are assumed or is this not possible with the information known in the declaration phase? Reading @jakemac53's explanation on how this is possible for constructors also let's me assume this is possible for const fields, since we should know both the import uri of the const field's library as well as the name of the global field.

@scheglov
Copy link
Contributor

During the declarations phase we cannot resolve identifiers that are not guaranteed to be names of types.
Because resolution @refAnnotation could change if the next macro will declare refAnnotation in this library.
Of course we technically can do this in the analyzer, but the result might be sometimes surprising.

My reading of @jakemac53 comment "you can't tie the constructor identifier to a specific AST node or element" - we don't know the element, but this does not matter because we can output the constructor name, and regardless what it is, it will be the same as the referenced by the annotation.

@helightdev
Copy link
Author

Ah okay I see the problem. So if I want to have declarative macros which are copying metadata, this will only work for const constructor invocations. Thanks for clarifying!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
Development

No branches or pull requests

4 participants