search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.09k stars 1.56k forks source link

ParameterElementImpl.defaultValueCode should be filled without AST resolution #46284

Open scheglov opened 3 years ago

scheglov commented 3 years ago

Currently it looks that we fill it only as a sub-product of resolving AST in ResolutionVisitor. But this is not correct, we should always provide default values, even when just element model was requested.

kevmoo commented 3 years ago

Is there a work-around for this in the short term, @scheglov ?

scheglov commented 3 years ago

The only one I can think of is to ask the corresponding resolved library, but this is expensive.

In the future I'd like to remove this getter, and replace it with Expression? get defaultValue. We already store default values in summaries, and have them as ConstVariableElement.constantInitializer, but don't export via API. Also offsets are not always set for tokens in these expressions, working on improving this.

scheglov commented 3 years ago

@kevmoo Could you clarify why you need defaultValueCode? Do you need just a fact that there is a default value, do you need the value as DartObject, do you need the raw unresolved code, do you need the resolved Expression?

kevmoo commented 3 years ago

Right now I just need the string. I wish I could get the string in constant expressions and annotations for instance.

On Thu, Jun 10, 2021, 17:43 Konstantin Scheglov @.***> wrote:

@kevmoo https://github.com/kevmoo Could you clarify why you need defaultValueCode? Do you need just a fact that there is a default value, do you need the value as DartObject, do you need the raw unresolved code, do you need the resolved Expression?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/46284#issuecomment-859179088, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEFCSPCRFL5C4KNMNWBQ3TSFL4TANCNFSM46IWCF7Q .

scheglov commented 3 years ago

If we will add Expression? get defaultValue to ParameterElement, and we have actual offsets for it, I think this will satisfy your use case. If you need the string, you will able to use substring from the full file content.

kevmoo commented 3 years ago

If we will add Expression? get defaultValue to ParameterElement, and we have actual offsets for it, I think this will satisfy your use case. If you need the string, you will able to use substring from the full file content.

@scheglov – give me an idea of the code I'd have to write at

https://github.com/google/json_serializable.dart/blob/f803b7509295d790f5d6580e879265b5444d7ce3/json_serializable/lib/src/utils.dart#L112-L115

scheglov commented 3 years ago

You need to be able to access content of files that produced this element model. If this element model is created from source files, you have access to them. If it was resynthesized from external summaries in google3, you cannot have exact code - it is not stored anywhere.

Then you would able to write something like

void example(
  ConstructorElement constructor,
  String Function(String path) getFileContent,
) {
  var defaultValueMap = <String, String>{};
  for (var parameter in constructor.parameters) {
    var defaultValue = parameter.defaultValue;
    if (defaultValue is Expression) {
      var content = getFileContent(constructor.source.fullName);
      var defaultValueCode = content.substring(
        defaultValue.offset,
        defaultValue.end,
      );
      defaultValueMap[parameter.name] = defaultValueCode;
    }
  }
}
kevmoo commented 3 years ago

@scheglov – not as pretty as the solution I have now, but I'd live.

scheglov commented 3 years ago

Well, practically what you have now either does not work - because you need to resolve the file before this data is available, or you do such file reading (to parse and resolve) plus much more (parse and resolve).

FilledStacks commented 3 years ago

Currently defaultValueCode returns null, always. There's no mention of this change and it's breaking generator packages that depend on getting the default value back.

@kevmoo did the above code work in the latest version for your package?

kevmoo commented 3 years ago

@FilledStacks – I haven't published a version that uses defaultValueCode

@scheglov – could we consider pumping the priority here, since we have someone else broken?

CC @devoncarew

scheglov commented 3 years ago

Hm... I wonder if there might be something else going on. I actually see default values for parameters.

I initially thought that we removed it in 097a953caed92e4787c9300c161f38cba4fad099. But I see that we return defaultValueCode in https://github.com/dart-lang/sdk/blob/aa0e8a5ca6e033d19fe9fb1ac2aeaaa434943d46/pkg/analyzer/lib/src/dart/element/element.dart#L1634-L1637

I tried following example.

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

main() async {
  ResourceProvider resourceProvider = PhysicalResourceProvider.INSTANCE;
  var analyzer = '/Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer';
  var collection = AnalysisContextCollectionImpl(
    includedPaths: [analyzer],
    resourceProvider: resourceProvider,
    sdkPath: '/Users/scheglov/Applications/dart-sdk',
  );

  var session = collection.contextFor(analyzer).currentSession;

  {
    var unitResult = await session.getLibraryByUri2(
        'package:analyzer/src/context/builder.dart') as LibraryElementResult;
    var libraryElement = unitResult.element;
    print(libraryElement);
    var classElement =
        libraryElement.definingCompilationUnit.getType('ContextBuilder')!;
    var parameterElement = classElement
        .getMethod('createWorkspace')!
        .parameters
        .firstWhere(
            (element) => element.name == 'lookForBazelBuildFileSubstitutes');
    print(parameterElement.defaultValueCode);
    print(parameterElement);
  }

  {
    var unitResult = await session.getLibraryByUri2(
            'package:analyzer/src/dart/analysis/analysis_context_collection.dart')
        as LibraryElementResult;
    var libraryElement = unitResult.element;
    print(libraryElement);
    var classElement = libraryElement.definingCompilationUnit
        .getType('AnalysisContextCollectionImpl')!;
    var parameterElement = classElement.unnamedConstructor!.parameters
        .firstWhere((element) => element.name == 'enableIndex');
    print(parameterElement.defaultValueCode);
    print(parameterElement);
  }
}

This code prints corresponding default values, and note that it requests only LibraryElement, and does not resolve AST.

true
{bool lookForBazelBuildFileSubstitutes = true}

false
{bool enableIndex = false}

It works for d97bd979570c4aacde322d8b04256ce477aa9729 when analyzer 1.7.0 was published, and on dea0488efedbd10a913489e35f52d445160c92b9 (current master) as well.

So, I guess, we need more information to understand what does not work, and when - which APIs invoked, what is code under analysis.

kevmoo commented 3 years ago

I'm happy to help debug!

On Wed, Jun 16, 2021 at 9:02 PM Konstantin Scheglov < @.***> wrote:

Hm... I wonder if there might be something else going on. I actually see default values for parameters.

import 'package:analyzer/dart/analysis/results.dart';import 'package:analyzer/file_system/file_system.dart';import 'package:analyzer/file_system/physical_file_system.dart';import 'package:analyzer/src/dart/analysis/analysis_context_collection.dart'; main() async { ResourceProvider resourceProvider = PhysicalResourceProvider.INSTANCE; var analyzer = '/Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer'; var collection = AnalysisContextCollectionImpl( includedPaths: [analyzer], resourceProvider: resourceProvider, sdkPath: '/Users/scheglov/Applications/dart-sdk', );

var session = collection.contextFor(analyzer).currentSession;

{ var unitResult = await session.getLibraryByUri2( 'package:analyzer/src/context/builder.dart') as LibraryElementResult; var libraryElement = unitResult.element; print(libraryElement); var classElement = libraryElement.definingCompilationUnit.getType('ContextBuilder')!; var parameterElement = classElement .getMethod('createWorkspace')! .parameters .firstWhere( (element) => element.name == 'lookForBazelBuildFileSubstitutes'); print(parameterElement.defaultValueCode); print(parameterElement); }

{ var unitResult = await session.getLibraryByUri2( 'package:analyzer/src/dart/analysis/analysis_context_collection.dart') as LibraryElementResult; var libraryElement = unitResult.element; print(libraryElement); var classElement = libraryElement.definingCompilationUnit .getType('AnalysisContextCollectionImpl')!; var parameterElement = classElement.unnamedConstructor!.parameters .firstWhere((element) => element.name == 'enableIndex'); print(parameterElement.defaultValueCode); print(parameterElement); } }

This code prints corresponding default values, and note that it requests only LibraryElement, and does not resolve AST.

true {bool lookForBazelBuildFileSubstitutes = true}

false {bool enableIndex = false}

It works for d97bd97 https://github.com/dart-lang/sdk/commit/d97bd979570c4aacde322d8b04256ce477aa9729 when analyzer 1.7.0 was published, and on dea0488 https://github.com/dart-lang/sdk/commit/dea0488efedbd10a913489e35f52d445160c92b9 (current master) as well.

So, I guess, we need more information to understand what does not work, and when - which APIs invoked, what is code under analysis.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/46284#issuecomment-862903796, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEFCVP523OESEIBOAR55TTTFXVTANCNFSM46IWCF7Q .

FilledStacks commented 3 years ago

@scheglov thank you for the example. I also like this setup you have for testing that out. I've been actually struggling to test. I have to run the generator in the source code to get my output so debugging is quite hard.

I'll try get a more succinct example for you to reproduce the null on my side. I have a hour or two later where I can tackle it. Thank you again.

FilledStacks commented 3 years ago

@scheglov what we're using is the function below. I omitted some code but you can see the entire file here for the parameter resolving 


  Future<RouteConfig> resolve(ConstantReader stackedRoute) async {
    final routeConfig = RouteConfig();
    ...
    final constructor = classElement.unnamedConstructor;

    var params = constructor?.parameters;
    if (params?.isNotEmpty == true) {
      if (constructor!.isConst &&
          params!.length == 1 &&
          toDisplayString(params.first.type) == 'Key') {
        routeConfig.hasConstConstructor = true;
      } else {
        final paramResolver = RouteParameterResolver(_importResolver);
        routeConfig.parameters = [];
        for (ParameterElement p in constructor.parameters) {
          routeConfig.parameters?.add(await paramResolver.resolve(p));
        }
      }
    }

    return routeConfig;
  }

 // Resolve function used is shows

  Future<RouteParamConfig> resolve(ParameterElement parameterElement) async {
    ...
    return RouteParamConfig(
        ...
        defaultValueCode: parameterElement.defaultValueCode,
        imports: _importResolver.resolveAll(paramType));
  }

As you see we do:

  1. classElement.unnamedConstructor
  2. constructor?.parameters
  3. parameterElement.defaultValueCode is null at this point.

All of this is being fed through the ConstantReader that's read throught GeneratorForAnnotation from the source_gen package. Do you think it might be something on their side that's not working?

scheglov commented 3 years ago

Hm... Maybe there is really no default value for this parameter? Again, I need something that I can run to reproduce this. If there is such example, I could add some logging into package:analyzer and track it down. But it seems not practical for me try to discover how and what to run, on my own.

kevmoo commented 3 years ago

Here's the flaky output – https://github.com/google/json_serializable.dart/runs/2866759454?check_suite_focus=true

I can help you debug it more on Monday

FilledStacks commented 3 years ago

completely agree. I have never written a stand alone example without running the generator. I will link am example project at home that uses the generator where the value is null.

I hope that will work.

Get Outlook for Androidhttps://aka.ms/AAb9ysg

From: Kevin Moore @.> Sent: Sunday, June 20, 2021 4:13:23 AM To: dart-lang/sdk @.> Cc: Dane Mackier @.>; Mention @.> Subject: Re: [dart-lang/sdk] ParameterElementImpl.defaultValueCode should be filled without AST resolution (#46284)

Here's the flaky output – https://github.com/google/json_serializable.dart/runs/2866759454?check_suite_focus=true

I can help you debug it more on Monday

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/dart-lang/sdk/issues/46284#issuecomment-864488930, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA3M72S6SGAQC6RXQJOORP3TTVFEHANCNFSM46IWCF7Q.

FilledStacks commented 3 years ago

@scheglov Here is an example where you can run the code and you'll see the generated code returns no value for the default parameter in app.router.dart.

If you can give me a verbal guide on how to create a better example for you based on this then I'd be more than happy to provide it to you.

Currently we're using

 build: ^2.0.0
 source_gen: ^1.0.0
 analyzer: ^1.3.0

For our functionality. Specifically the GeneratorForAnnotation Generator to read the config of what we need to generate. I hope this helps. I'd like to get the problem solved. I've tried with latest packages and it still doesn't work. I can't downgrade because it clashes and causes pubspec dependency hell for the devs using our package (us included).

scheglov commented 3 years ago

I have this example cloned. What should I run? I tried to run flutter build apk and it finished without any exception. So, I don't know where to look. I tried to use dependency overrides in packages/stacked_generator/pubspec.yaml, but flutter build apk still runs without exceptions.

dependency_overrides:
  _fe_analyzer_shared:
    path: /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/_fe_analyzer_shared
  analyzer:
    path: /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer
  meta:
    path: /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/meta
kevmoo commented 3 years ago

I've noticed it's repeated builds that break. The output changes.

Often with a clean build, everything is fine. It's as though some code paths are skipped in an incremental build (maybe that hydrates some data structures).

On Mon, Jun 21, 2021 at 11:34 AM Konstantin Scheglov < @.***> wrote:

I have this example cloned. What should I run? I tried to run flutter build apk and it finished without any exception. So, I don't know where to look. I tried to use dependency overrides in packages/stacked_generator/pubspec.yaml, but flutter build apk still runs without exceptions.

dependency_overrides: _fe_analyzer_shared: path: /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/_fe_analyzer_shared analyzer: path: /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer meta: path: /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/meta

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/46284#issuecomment-865255485, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEFCREENZFXDLPDUPUSOLTT6AZ7ANCNFSM46IWCF7Q .

scheglov commented 3 years ago

OK, I think what I should run is flutter pub run build_runner build --delete-conflicting-outputs. But I still don't understand what should I look at. The generator expects default values for some parameter in some method / constructor of some class? Which one?

scheglov commented 3 years ago

I added some debug output into your generator, but I get only 3 parameters, and looking into code, there are no default values for them in code.

$ flutter pub run build_runner build --delete-conflicting-outputs
<cut>
[WARNING] stacked_generator:stackedRouterGenerator on lib/app/app.dart:
[parameterElement.defaultValueCode: null][parameterElement: {Key? key}][enclosing: DetailsView DetailsView({Key? key, required String name})][enclosing2: class DetailsView extends StatelessWidget]
[WARNING] stacked_generator:stackedRouterGenerator on lib/app/app.dart:
[parameterElement.defaultValueCode: null][parameterElement: {required String name}][enclosing: DetailsView DetailsView({Key? key, required String name})][enclosing2: class DetailsView extends StatelessWidget]
[WARNING] stacked_generator:stackedRouterGenerator on lib/app/app.dart:
[parameterElement.defaultValueCode: null][parameterElement: {Key? key}][enclosing: ExampleFormView ExampleFormView({Key? key})][enclosing2: class ExampleFormView extends StatelessWidget with $ExampleFormView]
scheglov commented 3 years ago

https://dart-review.googlesource.com/c/sdk/+/204485 fixes the issue for json_serializable.

FilledStacks commented 3 years ago

@scheglov sorry for the sloppy example with instructions. I was squashing it in between some calls.

The link above looks promising. Let me see if it works. Will be back in few minutes with an answer.

FilledStacks commented 3 years ago

Running into a lot of dependency issues using the latest version on master from git that has the changes above in it. 

Because source_gen >=1.0.1 depends on analyzer ^1.7.0 and source_gen >=1.0.0 <1.0.1 depends on analyzer ^1.1.0, source_gen >=1.0.0 requires analyzer from hosted.
So, because stacked_generator depends on both source_gen ^1.0.0 and analyzer from git, version solving failed.
Running "flutter pub get" in stacked_generator...                       
pub get failed (1; So, because stacked_generator depends on both source_gen ^1.0.0 and analyzer from git, version solvingfailed.)

had to override meta 

dependency_overrides:
  meta: ^1.4.0

Also tried with source gen from git.

source_gen:
    # ^1.0.0
    git:
      url: https://github.com/dart-lang/source_gen.git
      path: source_gen/

I'll pull down the repo's and point to local packages and reference that. I will be back, probably later today, 3-4 hours from now orrrrr tomorrow sometime.

scheglov commented 3 years ago

For json_serializable.dart I had to use following overrides:

dependency_overrides:
  _fe_analyzer_shared:
    path: /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/_fe_analyzer_shared
  analyzer:
    path: /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer
  meta:
    path: /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/meta
kevmoo commented 3 years ago

ditto

On Mon, Jun 28, 2021 at 4:13 PM Konstantin Scheglov < @.***> wrote:

For json_serializable.dart I had to use following overrides:

dependency_overrides: _fe_analyzer_shared: path: /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/_fe_analyzer_shared analyzer: path: /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/analyzer meta: path: /Users/scheglov/Source/Dart/sdk.git/sdk/pkg/meta

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/46284#issuecomment-870106932, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEFCQG3LCB5COFORMVTK3TVD6ZJANCNFSM46IWCF7Q .