Nicholas Shahan (Gerrit)

unread,

Apr 20, 2026, 12:53:02 PM (2 days ago) Apr 20

to Jens Johansen, Johnni Winther, dart-...@luci-project-accounts.iam.gserviceaccount.com, Alexander Markov, dart-fe-te...@google.com, rev...@dartlang.org

Attention needed from Jens Johansen and Johnni Winther

Nicholas Shahan added 4 comments

File pkg/frontend_server/lib/compute_kernel.dart

Line 395, Patchset 6 (Latest):

        // trackWidgetCreation is in TargetFlags.
        "trackWidgetCreation=$trackWidgetCreation",
        // includeUnsupportedPlatformLibraryStubs is in TargetFlags.
        "includeUnsupportedPlatformLibraryStubs="
            "$includeUnsupportedPlatformLibraryStubs",

Nicholas Shahan . unresolved

I don't understand the two comments here. If this information is important could you elaborate them a little more?

File pkg/kernel/lib/target/targets.dart

Line 337, Patchset 6 (Latest): /// This is used to transforming the libraries, but not for instance

Nicholas Shahan . unresolved

```suggestion
  /// This is used to transform the libraries, but not for instance
```

Line 620, Patchset 6 (Latest): bool isModularCompatibleWith(Target other) => true;

Nicholas Shahan . unresolved

Do we really want this default implementation? Wouldn't this be better left unimplemented to force subclasses to verify the compatibility of their targets?

I don't see an override implementation in DevCompilerTarget in this CL. Would we really want it inheriting this default behavior?

Line 622, Patchset 6 (Latest): void updateModularCompatibilityAs(Target other) {}

Nicholas Shahan . unresolved

Same here. Should this be abstract as well?

Open in Gerrit

Related details

Attention is currently required from:

Jens Johansen
Johnni Winther

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Jens Johansen (Gerrit)

unread,

Apr 21, 2026, 2:29:48 AM (yesterday) Apr 21

to Johnni Winther, Nicholas Shahan, dart-...@luci-project-accounts.iam.gserviceaccount.com, Alexander Markov, dart-fe-te...@google.com, rev...@dartlang.org

Attention needed from Johnni Winther and Nicholas Shahan

Jens Johansen voted and added 4 comments

Votes added by Jens Johansen

Commit-Queue

+1

4 comments

File pkg/frontend_server/lib/compute_kernel.dart

Line 395, Patchset 6: // trackWidgetCreation is in TargetFlags.


        "trackWidgetCreation=$trackWidgetCreation",
        // includeUnsupportedPlatformLibraryStubs is in TargetFlags.
        "includeUnsupportedPlatformLibraryStubs="
            "$includeUnsupportedPlatformLibraryStubs",

Nicholas Shahan . resolved

I don't understand the two comments here. If this information is important could you elaborate them a little more?

Jens Johansen

These are "tags" used to verify that we can reuse the previous incremental state. The "trackWidgetCreation" part is old, I added "includeUnsupportedPlatformLibraryStubs" (although technically not necessary to fix the issue at hand) and I thought it best to also add where they came from so I added these comments trying to tell the reader that it is used in `TargetFlags` created previously as

```
  TargetFlags targetFlags = new TargetFlags(
    trackWidgetCreation: trackWidgetCreation,
    includeUnsupportedPlatformLibraryStubs:
        includeUnsupportedPlatformLibraryStubs,
  );
```

These could probably be moved into the `isModularCompatibleWith` commented on below, but that would be more of a refactor than a fix I think, and thus - to me - belongs in a different CL.

File pkg/kernel/lib/target/targets.dart

Line 337, Patchset 6: /// This is used to transforming the libraries, but not for instance

Nicholas Shahan . resolved

```suggestion
  /// This is used to transform the libraries, but not for instance
```

Jens Johansen

Done

Line 620, Patchset 6: bool isModularCompatibleWith(Target other) => true;

Nicholas Shahan . resolved

Do we really want this default implementation? Wouldn't this be better left unimplemented to force subclasses to verify the compatibility of their targets?
I don't see an override implementation in DevCompilerTarget in this CL. Would we really want it inheriting this default behavior?

Jens Johansen

Well.

Before there was no check. Adding the check will therefore be a no-op for the Targets when this defaults to `true` (and doing nothing in the update version). That makes sense to me.

As mentioned above I agree that a refactor could move at least some of the tags into this method instead of having it as tags. To me that belongs in another CL though.

Line 622, Patchset 6: void updateModularCompatibilityAs(Target other) {}

Nicholas Shahan . resolved

Same here. Should this be abstract as well?

Jens Johansen

See above.

Open in Gerrit

Related details

Attention is currently required from:

Johnni Winther
Nicholas Shahan

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Johnni Winther (Gerrit)

unread,

Apr 21, 2026, 4:09:28 AM (yesterday) Apr 21

to Jens Johansen, Nicholas Shahan, dart-...@luci-project-accounts.iam.gserviceaccount.com, Alexander Markov, dart-fe-te...@google.com, rev...@dartlang.org

Attention needed from Jens Johansen and Nicholas Shahan

Johnni Winther voted and added 2 comments

Votes added by Johnni Winther

Code-Review

+1

2 comments

File pkg/kernel/lib/target/targets.dart

Line 620, Patchset 7 (Latest): bool isModularCompatibleWith(Target other) => true;

Johnni Winther . unresolved

Maybe `hasModularCompatibilityWith` or `isModularlyCompatibleWith`.

Line 620, Patchset 7 (Latest):

  bool isModularCompatibleWith(Target other) => true;

  void updateModularCompatibilityAs(Target other) {}
}

Johnni Winther . unresolved

Add documentation for these.

Open in Gerrit

Related details

Attention is currently required from:

Jens Johansen
Nicholas Shahan

Submit Requirements:

Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Jens Johansen (Gerrit)

unread,

Apr 21, 2026, 9:56:41 AM (yesterday) Apr 21

to Johnni Winther, Nicholas Shahan, dart-...@luci-project-accounts.iam.gserviceaccount.com, Alexander Markov, dart-fe-te...@google.com, rev...@dartlang.org

Attention needed from Nicholas Shahan

Jens Johansen voted and added 2 comments

Votes added by Jens Johansen

Commit-Queue

+1

2 comments

File pkg/kernel/lib/target/targets.dart

Line 620, Patchset 7: bool isModularCompatibleWith(Target other) => true;

Johnni Winther . resolved

Maybe `hasModularCompatibilityWith` or `isModularlyCompatibleWith`.

Jens Johansen

Done

Line 620, Patchset 7:

  bool isModularCompatibleWith(Target other) => true;

  void updateModularCompatibilityAs(Target other) {}
}

Johnni Winther . resolved

Add documentation for these.

Jens Johansen

Done

Open in Gerrit

Related details

Attention is currently required from:

Nicholas Shahan

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Nicholas Shahan (Gerrit)

unread,

Apr 21, 2026, 11:56:11 AM (23 hours ago) Apr 21

to Jens Johansen, Johnni Winther, dart-...@luci-project-accounts.iam.gserviceaccount.com, Alexander Markov, dart-fe-te...@google.com, rev...@dartlang.org

Attention needed from Jens Johansen

Nicholas Shahan voted and added 1 comment

Votes added by Nicholas Shahan

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 8 (Latest):

Nicholas Shahan . resolved

Thanks for the extra explanations.

Open in Gerrit

Related details

Attention is currently required from:

Jens Johansen

Submit Requirements:

Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Jens Johansen (Gerrit)

unread,

2:26 AM (9 hours ago) 2:26 AM

to Nicholas Shahan, Johnni Winther, dart-...@luci-project-accounts.iam.gserviceaccount.com, Alexander Markov, dart-fe-te...@google.com, rev...@dartlang.org

Jens Johansen voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

dart-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)

unread,

2:26 AM (9 hours ago) 2:26 AM

to Jens Johansen, Nicholas Shahan, Johnni Winther, Alexander Markov, dart-fe-te...@google.com, rev...@dartlang.org

dart-...@luci-project-accounts.iam.gserviceaccount.com submitted the change

Change information

Commit message:

Reapply "[kernel/cfe/etc] Split outline transformation into performOutlineTransformations and performOutlineComponentOperations"

This reverts commit 264098c85fa326bcc5c88cd4ccd7628980a58c14.

Currently outline transformations aren't run via the incremental
compiler which causes problems in
https://dart-review.googlesource.com/c/sdk/+/491702 which fixes it by
calling the current transformation in the incremental compiler. This
calls it twice though (because it's run again in
`frontend_server/lib/compute_kernel.dart`, but removing it there doesn't
work because a filtering is done which doesn't apply through the
incremental compiler.

This CL splits up the outline transformation stage into a call that can
actually transform the libraries and one that can do the filtering,
which should fix the issue.

Original CL was reverted because it caused errors in google3. The
original CL is in patchset 1. The error has been reproduced and
recreated in a test added in patchset 2. The fix is in patchset 3.

The dwds failure @
https://github.com/dart-lang/webdev/actions/runs/24516059718/job/71660245069
has been verified as fixed as well.

The problem was this:

Previously outline transformations were not run by the incremental
compiler, but only outside. When it was moved to the incremental
compiler it had on old - outdated - `target` which for the ddc/dart2js
summary target would hold a list of source files that it was initially
created with, not the ones currently being compiled. This meant that the
transformation step that removed "unrelated" libraries actually removed
the newly compiled libraries instead. It could cause one of two issues:
1) Empty output: With no overlap between the combined output of the
compile and the sources of the first compile (i.e. the ones in the
outdated `target`) all libraries were filtered out. If a later
compile was given this as a summary input the compile could fail with
a file not found error because the given summary - which should
contain the missing library didn't.
2) Non-empty output: With an overlap between the combined output of the
compile and the sources of the first compile only the overlap would
be included. In practise this would mean that the output would be a
(potentially partial) copy of the first compile. If then a later
compile was given both the summary from the first compile and the
output with the copy it would throw when loading because it got the
same library from two different summaries.

Change-Id: If712663acdbd7d25ccb3beab54a7efac0c0b0568

Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/496181

Reviewed-by: Nicholas Shahan <nsh...@google.com>

Reviewed-by: Johnni Winther <johnni...@google.com>

Commit-Queue: Jens Johansen <je...@google.com>

Files:

M pkg/compiler/lib/src/kernel/dart2js_target.dart
M pkg/front_end/lib/src/api_unstable/modular_incremental_compilation.dart
M pkg/front_end/lib/src/base/incremental_compiler.dart
M pkg/front_end/lib/src/kernel_generator_impl.dart
M pkg/frontend_server/lib/compute_kernel.dart
A pkg/frontend_server/test/compute_kernel_test.dart
M pkg/kernel/lib/target/targets.dart

Change size: L

Delta: 7 files changed, 375 insertions(+), 7 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Johnni Winther, +1 by Nicholas Shahan

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[L] Change in dart/sdk[main]: Reapply "[kernel/cfe/etc] Split outline transformation into performOu...

Nicholas Shahan (Gerrit)

Nicholas Shahan added 4 comments

Related details

Jens Johansen (Gerrit)

Jens Johansen voted and added 4 comments

Votes added by Jens Johansen

4 comments

Related details

Johnni Winther (Gerrit)

Johnni Winther voted and added 2 comments

Votes added by Johnni Winther

2 comments

Related details

Jens Johansen (Gerrit)

Jens Johansen voted and added 2 comments

Votes added by Jens Johansen

2 comments

Related details

Nicholas Shahan (Gerrit)

Nicholas Shahan voted and added 1 comment

Votes added by Nicholas Shahan

1 comment

Related details

Jens Johansen (Gerrit)

Jens Johansen voted Commit-Queue+2

Related details

dart-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)

dart-...@luci-project-accounts.iam.gserviceaccount.com submitted the change

Change information