Skip to content

Introduce the Mustache AOT compiler #2651

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

Merged
merged 11 commits into from
May 24, 2021
Merged

Conversation

srawlins
Copy link
Member

@srawlins srawlins commented May 18, 2021

When the context types and the templates are known ahead-of-time, we can do much better than Mustachio's runtime renderers, and instead compile exceedingly straightforward render code.

This PR includes the code generator for AOT renderers, but does not wire it up to dartdoc yet.

This PR includes a README which describes the design of the generated files, but does not yet describe the design of the generator. 😄

  • The generated runtime renderers are now in *.runtime_renderers.dart instead of .renderers.dart, to distinguish. These renderers are still in use for all cases of rendering into templates.
  • The generated aot-compiled renderers are *.aot_renderers_for_html.dart and *.aot_renderers_for_md.dart.
  • Each of these three files is generated by build_runner.
  • The @Renderer annotations need a new argument: the (approximate) base file name of the template file. Again, we don't use the AOT renderers yet, but the annotations still need to be legal, and still generate the renderers.
  • Refactored tests to share a common base.
  • Move RendererSpec from tool/ to lib/src/mustachio/annotations.dart, so it can be shared between builders.

@google-cla google-cla bot added the cla: yes Google CLA check succeeded. label May 18, 2021
@coveralls
Copy link

coveralls commented May 18, 2021

Coverage Status

Coverage increased (+0.02%) to 58.23% when pulling 746d2a2 on srawlins:aot-compiler into 48e42dc on dart-lang:master.

@srawlins srawlins requested a review from jcollins-g May 19, 2021 16:29
@srawlins
Copy link
Member Author

This is a big PR. I can break it up into a few smaller if desired, like moving RendererSpec to a shared location, and extracting yet-to-be-shared test common code to a shared file. The smaller ones will just have changes that don't make sense without this one as follow up.


/// Specifies information for generating both a runtime-interpreted Mustache
/// renderer and a pre-compiled Mustache renderer for a [context] object, using
/// a Mustache template at [templateUri]
Copy link
Contributor

Choose a reason for hiding this comment

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

period at end of sentence?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

/// The unparsed, string form of the URI of the _standard_ Markdown template.
///
/// This represents the Mustache template that dartdoc uses out-of-the-box to
/// render the [context] object while generating documentation in HTML.
Copy link
Contributor

Choose a reason for hiding this comment

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

in markdown

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

import 'package:analyzer/dart/element/element.dart';

/// The build package Asset for a copy of the Renderer annotation for tests.
const annotationsAsset = {
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit more explanation as to why this is needed (and possibly, some cross linking saying to update this when the Renderer annotation gets modified) seems prudent. I had forgotten about this bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

writeln('buffer.writeln();');
} else {
content = content.replaceAll("'", "\\'").replaceAll(r'$', r'\\$');
if (RegExp('^[ \\n]+\$').hasMatch(content)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest using a private static for the RegExp instead of constructing it each time, depending on how Dart handles it these constructions could be expensive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch.

// Blank lines happen a lot; just indicate them as such.
writeln('buffer.writeln();');
} else {
content = content.replaceAll("'", "\\'").replaceAll(r'$', r'\\$');
Copy link
Contributor

Choose a reason for hiding this comment

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

Escaping single quotes does not work in multi-line dart strings. It won't break your program because they're no longer three literal single quotes in a row, but the output will be mangled. To handle this correctly you'll need to split content on a triple-single-quote separator and write the separators out in between the sections. I might also suggest that you use raw strings for your multi-liners, thus eliminating the need for substitutions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it will eliminate some of the need for substitutions, depending on how you want to handle the whitespace writes you might still need them there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate with an example? I'm not sure I follow what input would not be properly escaped...

Copy link
Contributor

Choose a reason for hiding this comment

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

Went back to my test code and for some reason I thought you were using raw multiline strings but you were not, so my original comment was wrong.

However, not using raw multiline strings means you will need to escape any backslashes in the output or otherwise a literal \f in the template, for example, will not translate across. So one or the other will need to be handled I think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good catch. Done.

_VariableLookup(this.type, this.name);
}

extension<T> on List<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Had no idea you could do extension<T> but I guess it makes sense!

Copy link
Member Author

Choose a reason for hiding this comment

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

yes very neat!

import 'package:dartdoc/src/generator/template_data.dart';
import 'foo.dart';

String renderFoo(Foo context0) {
Copy link
Member

Choose a reason for hiding this comment

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

Random one-off: could/would we pass in a StringSink here instead of creating a buffer.

Then you have the option of doing streaming writes later? Maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a TODO

@srawlins
Copy link
Member Author

Oh shwew and CI is good. Thanks!

@srawlins srawlins merged commit 18e648a into dart-lang:master May 24, 2021
@srawlins srawlins deleted the aot-compiler branch May 24, 2021 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants