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

mustachio: Separate out the context stack LUB type calculation #3730

Merged
merged 1 commit into from
Mar 20, 2024
Merged

mustachio: Separate out the context stack LUB type calculation #3730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions lib/src/generator/templates.aot_renderers_for_html.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@
// To change the contents of this library, make changes to the builder source
// files in the tool/mustachio/ directory.

// There are a few deduplicated render functions which are generated but not
// used.
// Some deduplicated render functions are generated but not used.
// TODO(srawlins): Detect these and do not write them.
// ignore_for_file: unused_element
// Sometimes we enter a new section which triggers creating a new variable, but
Expand Down
3 changes: 1 addition & 2 deletions test/mustachio/foo.aot_renderers_for_html.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@
// To change the contents of this library, make changes to the builder source
// files in the tool/mustachio/ directory.

// There are a few deduplicated render functions which are generated but not
// used.
// Some deduplicated render functions are generated but not used.
// TODO(srawlins): Detect these and do not write them.
// ignore_for_file: unused_element
// Sometimes we enter a new section which triggers creating a new variable, but
Expand Down
69 changes: 46 additions & 23 deletions tool/mustachio/codegen_aot_compiler.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ import 'package:dartdoc/src/type_utils.dart';
import 'package:meta/meta.dart';
import 'package:path/path.dart' as path;

/// A list of [_VariableLookup]s used by a renderer constitutes a "context
/// stack".
typedef ContextStack = List<_VariableLookup>;

/// Compiles all templates specified in [specs] into a Dart library containing
/// a renderer for each template.
Future<String> compileTemplatesToRenderers(
Expand Down Expand Up @@ -62,8 +66,7 @@ Future<String> compileTemplatesToRenderers(
// To change the contents of this library, make changes to the builder source
// files in the tool/mustachio/ directory.

// There are a few deduplicated render functions which are generated but not
// used.
// Some deduplicated render functions are generated but not used.
// TODO(srawlins): Detect these and do not write them.
// ignore_for_file: unused_element
// Sometimes we enter a new section which triggers creating a new variable, but
Expand Down Expand Up @@ -116,27 +119,23 @@ Future<Map<_AotCompiler, String>> _deduplicateRenderers(
continue;
}
var firstCompiler = compilers.first;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the codegen is kinda confusing. Why do we use the first compiler's _syntaxTree and _buildData when making the lubCompiler? Also, perhaps move this variable down there, since it's not being used for contextStacksLength anymore

All the other changes seem okay to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, apologies for this not being well documented, I do plan on sending out docs next.

OK, so in this section, we are looking at the "partial compilers" that have been generated for each partial file path. So if one template referred to the "_foo.html" partial with {{ >foo }} and another template also referred to that partial file with {{ >foo }}, then we have to partial compilers, and we're trying to deduplicate them. The reason they are initially separate and why we might not be able to deduplicate is that the variables in scope might be different from one calling template to another.

OK, so the variables might be different, but what is always common between these compilers (for the same partial path) is that they parse out to the same syntax tree (since its the same file on disk). And the _buildData is also just a common grab bag of data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see! Okay, that makes a lot of sense. Yeah, some extra documentation on that would be great :)

var contextStacksLength = firstCompiler._usedContexts.length;
if (compilers.any((c) => c._usedContexts.length != contextStacksLength)) {
// The stack lengths are different, it is impossible to deduplicate such
// partial renderers.
var contextStacks = compilers.map((c) => c._usedContextStack).toList();
var contextStackTypes = typeSystem.contextStackLub(contextStacks);
if (contextStackTypes == null) {
// The stack lengths are different; it is impossible to fully deduplicate
// such partial renderers.
continue;
}
// The new list of context types, each of which is the LUB of the associated
// context type of each of the compilers.
var contextStackTypes = <InterfaceType>[];
for (var i = 0; i < contextStacksLength; i++) {
var types = compilers.map((c) => c._usedContextStack[i].type);
var lubType = types.fold<DartType>(types.first,
(value, type) => typeSystem.leastUpperBound(value, type))
as InterfaceType;
contextStackTypes.add(lubType);
}

// Each of the render functions generated by a compiler for this asset can
// be replaced by a more generic renderer which accepts the LUB types. The
// body of each replaced renderer can perform a simple redirect.
// body of each replaced renderer can perform a simple redirect to the more
// generic renderer.
var rendererName = filePath.replaceAll('.', '_').replaceAll('/', '_');

// The names of the renderers which are being replaced all include some
// reference to the template/partial which referred to them; we must create
// a new renderer name from scratch.
var lubCompiler = _AotCompiler._(
contextStackTypes.first,
'_deduplicated_$rendererName',
Expand Down Expand Up @@ -250,12 +249,12 @@ class _AotCompiler {
final Map<_AotCompiler, String> _compiledPartials = {};

/// The current stack of context objects (as variable lookups).
final List<_VariableLookup> _contextStack;
final ContextStack _contextStack;

/// The set of context objects which are ultimately used by this compiler.
final Set<_VariableLookup> _usedContexts = {};

List<_VariableLookup> get _usedContextStack =>
ContextStack get _usedContextStack =>
[..._contextStack.where(_usedContexts.contains)];

/// A counter for naming partial render functions.
Expand All @@ -276,7 +275,7 @@ class _AotCompiler {
String rendererName,
String templatePath,
_BuildData buildData, {
List<_VariableLookup> contextStack = const [],
ContextStack contextStack = const [],
}) async {
var template =
await File(path.join(buildData._root, templatePath)).readAsString();
Expand All @@ -292,7 +291,7 @@ class _AotCompiler {
this._templatePath,
this._syntaxTree,
this._buildData, {
required List<_VariableLookup> contextStack,
required ContextStack contextStack,
}) : _contextStack = _rename(contextStack),
_contextNameCounter = contextStack.length;

Expand All @@ -301,7 +300,7 @@ class _AotCompiler {
///
/// This ensures that each renderer accepts a simple list of context objects
/// with predictable names.
static List<_VariableLookup> _rename(List<_VariableLookup> original) {
static ContextStack _rename(ContextStack original) {
var result = <_VariableLookup>[];
var index = original.length - 1;
for (var variable in original) {
Expand Down Expand Up @@ -391,7 +390,7 @@ class _AotCompiler {
class _BlockCompiler {
final _AotCompiler _templateCompiler;

final List<_VariableLookup> _contextStack;
final ContextStack _contextStack;

final Set<_VariableLookup> _usedContextTypes = {};

Expand Down Expand Up @@ -756,3 +755,27 @@ extension on StringBuffer {
return referencedElements;
}
}

extension on TypeSystem {
/// Returns a list of types which represents the LUB of each of the types in
/// the corresponding positions in [contextStacks].
List<InterfaceType>? contextStackLub(List<ContextStack> contextStacks) {
// The length of each and every context stack.
var contextStacksLength = contextStacks.first.length;
if (contextStacks.any((s) => s.length != contextStacksLength)) {
return null;
}

// The new list of context types, each of which is the LUB of the associated
// context type of each of the compilers.
var contextStackTypes = <InterfaceType>[];
for (var i = 0; i < contextStacksLength; i++) {
var types = contextStacks.map((s) => s[i].type);
var lubType =
types.fold<DartType>(types.first, leastUpperBound) as InterfaceType;
contextStackTypes.add(lubType);
}

return contextStackTypes;
}
}