Automatically URI encode asset keys

impeller/compiler/compiler.cc

@@ -464,13 +464,24 @@ const std::vector<std::string>& Compiler::GetIncludedFileNames() const {
   return included_file_names_;
 }
 
+// Escape `%` and `#` characters according to doc comment at
+// https://github.com/ninja-build/ninja/blob/master/src/depfile_parser.cc#L28
+static void EscapeString(std::string& str, std::stringstream& stream) {
+  for (auto it = str.begin(); it != str.end(); it++) {
+    if (*it == '%' || *it == '#') {
+      stream << '\\';
+    }
+    stream << *it;
+  }
+}
+
 static std::string JoinStrings(std::vector<std::string> items,
                                std::string separator) {
   std::stringstream stream;
   for (size_t i = 0, count = items.size(); i < count; i++) {
     const auto is_last = (i == count - 1);
 
-    stream << items[i];
+    EscapeString(items[i], stream);
     if (!is_last) {
       stream << separator;
     }
@@ -486,6 +497,7 @@ std::string Compiler::GetDependencyNames(std::string separator) const {
 
 std::unique_ptr<fml::Mapping> Compiler::CreateDepfileContents(
     std::initializer_list<std::string> targets_names) const {
+  // https://github.com/ninja-build/ninja/blob/master/src/depfile_parser.cc#L28
   const auto targets = JoinStrings(targets_names, " ");
   const auto dependencies = GetDependencyNames(" ");
 

impeller/compiler/compiler_test.cc

@@ -58,6 +58,37 @@ static std::string SLFileName(const char* fixture_name,
   return stream.str();
 }
 
+static std::string DepfileName(const char* fixture_name) {
+  std::stringstream stream;
+  stream << fixture_name << ".d";
+  return stream.str();
+}
+
+bool CompilerTest::ValidateDepfileEscaped(const char* fixture_name) const {
+  auto depfile_name = DepfileName(fixture_name);
+  auto mapping = std::make_unique<fml::FileMapping>(
+      fml::OpenFile(intermediates_directory_, depfile_name.c_str(), false,
+                    fml::FilePermission::kRead));
+
+  std::string contents(reinterpret_cast<char const*>(mapping->GetMapping()),
+                       mapping->GetSize());
+  bool escaped = false;
+  for (auto it = contents.begin(); it != contents.end(); it++) {
+    if (*it == '\\') {
+      escaped = true;
+    } else if (*it == '%' || *it == '#') {
+      if (!escaped) {
+        VALIDATION_LOG << "Unescaped character " << *it << " in depfile.";
+        return false;
+      }
+      escaped = false;
+    } else {
+      escaped = false;
+    }
+  }
+  return true;
+}
+
 bool CompilerTest::CanCompileAndReflect(const char* fixture_name,
                                         SourceType source_type) const {
   auto fixture = flutter::testing::OpenFixtureAsMapping(fixture_name);
@@ -160,6 +191,12 @@ bool CompilerTest::CanCompileAndReflect(const char* fixture_name,
     }
   }
 
+  auto mapping = compiler.CreateDepfileContents({fixture_name});
+  if (!fml::WriteAtomically(intermediates_directory_,
+                            DepfileName(fixture_name).c_str(), *mapping)) {
+    VALIDATION_LOG << "Could not write depfile.";
+    return false;
+  }
   return true;
 }
 

impeller/compiler/compiler_test.h

@@ -25,6 +25,8 @@ class CompilerTest : public ::testing::TestWithParam<TargetPlatform> {
       const char* fixture_name,
       SourceType source_type = SourceType::kUnknown) const;
 
+  bool ValidateDepfileEscaped(const char* fixture_name) const;
+
  private:
   fml::UniqueFD intermediates_directory_;
 

impeller/compiler/compiler_unittests.cc

@@ -65,6 +65,11 @@ TEST_P(CompilerTest, MustFailDueToMultipleLocationPerStructMember) {
   ASSERT_FALSE(CanCompileAndReflect("struct_def_bug.vert"));
 }
 
+TEST_P(CompilerTest, ShaderWithSpecialCharactersHasEscapedDepfile) {
+  ASSERT_TRUE(CanCompileAndReflect("sa\%m#ple.vert"));
+  ASSERT_TRUE(ValidateDepfileEscaped("sa\%m#ple.vert"));
+}
+
 #define INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P(suite_name)              \
   INSTANTIATE_TEST_SUITE_P(                                               \
       suite_name, CompilerTest,                                           \

impeller/fixtures/BUILD.gn

@@ -47,6 +47,7 @@ test_fixtures("file_fixtures") {
     "sample.tesc",
     "sample.tese",
     "sample.vert",
+    "sa%m#ple.vert",
     "struct_def_bug.vert",
     "table_mountain_nx.png",
     "table_mountain_ny.png",

impeller/fixtures/sa%m#ple.vert

@@ -0,0 +1,23 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "types.h"
+
+uniform UniformBufferObject {
+  Uniforms uniforms;
+} ubo;
+
+uniform sampler2D world;
+
+in vec2 inPosition;
+in vec3 inPosition22;
+in vec4 inAnotherPosition;
+in float stuff;
+
+out vec4 outStuff;
+
+void main() {
+  gl_Position =  ubo.uniforms.projection * ubo.uniforms.view * ubo.uniforms.model * vec4(inPosition22, 1.0) * inAnotherPosition;
+  outStuff = texture(world, inPosition);
+}

lib/ui/BUILD.gn

@@ -187,6 +187,7 @@ if (enable_unittests) {
     dart_main = "fixtures/ui_test.dart"
     fixtures = [
       "fixtures/DashInNooglerHat.jpg",
+      "fixtures/DashInNooglerHat%20WithSpace.jpg",
       "fixtures/Horizontal.jpg",
       "fixtures/Horizontal.png",
       "fixtures/hello_loop_2.gif",

lib/ui/fixtures/shaders/general_shaders/BUILD.gn

@@ -11,6 +11,7 @@ if (enable_unittests) {
     "blue_green_sampler.frag",
     "children_and_uniforms.frag",
     "functions.frag",
+    "functions%20with_space.frag",
     "no_builtin_redefinition.frag",
     "no_uniforms.frag",
     "simple.frag",

lib/ui/fixtures/shaders/general_shaders/functions%20with_space.frag

@@ -0,0 +1,34 @@
+#version 320 es
+
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+precision highp float;
+
+layout ( location = 0 ) out vec4 oColor;
+
+layout ( location = 0 ) uniform float a;  // should be 1.0
+
+float addA(float x) {
+  return x + a;
+}
+
+vec2 pairWithA(float x) {
+  return vec2(x, a);
+}
+
+vec3 composedFunction(float x) {
+  return vec3(addA(x), pairWithA(x));
+}
+
+float multiParam(float x, float y, float z) {
+  return x * y * z * a;
+}
+
+void main() {
+  float x = addA(0.0); // x = 0 + 1;
+  vec3 v3 = composedFunction(x); // v3 = vec3(2, 1, 1);
+  x = multiParam(v3.x, v3.y, v3.z); // x = 2 * 1 * 1 * 1;
+  oColor = vec4(0.0, x / 2.0, 0.0, 1.0); // vec4(0, 1, 0, 1);
+}

lib/ui/painting.dart

@@ -4077,13 +4077,18 @@ class FragmentProgram extends NativeFieldWrapperClass1 {
   /// compiler. The constructed object should then be reused via the [shader]
   /// method to create [Shader] objects that can be used by [Shader.paint].
   static Future<FragmentProgram> fromAsset(String assetKey) {
-    final FragmentProgram? program = _shaderRegistry[assetKey]?.target;
+    // The flutter tool converts all asset keys with spaces into URI
+    // encoded paths (replacing ' ' with '%20', for example). We perform
+    // the same encoding here so that users can load assets with the same
+    // key they have written in the pubspec.
+    final String encodedKey = Uri(path: Uri.encodeFull(assetKey)).path;
+    final FragmentProgram? program = _shaderRegistry[encodedKey]?.target;
     if (program != null) {
       return Future<FragmentProgram>.value(program);
     }
     return Future<FragmentProgram>.microtask(() {
-      final FragmentProgram program = FragmentProgram._fromAsset(assetKey);
-      _shaderRegistry[assetKey] = WeakReference<FragmentProgram>(program);
+      final FragmentProgram program = FragmentProgram._fromAsset(encodedKey);
+      _shaderRegistry[encodedKey] = WeakReference<FragmentProgram>(program);
       return program;
     });
   }
@@ -5955,9 +5960,14 @@ class ImmutableBuffer extends NativeFieldWrapperClass1 {
   ///
   /// Throws an [Exception] if the asset does not exist.
   static Future<ImmutableBuffer> fromAsset(String assetKey) {
+    // The flutter tool converts all asset keys with spaces into URI
+    // encoded paths (replacing ' ' with '%20', for example). We perform
+    // the same encoding here so that users can load assets with the same
+    // key they have written in the pubspec.
+    final String encodedKey = Uri(path: Uri.encodeFull(assetKey)).path;
     final ImmutableBuffer instance = ImmutableBuffer._(0);
     return _futurize((_Callback<int> callback) {
-      return instance._initFromAsset(assetKey, callback);
+      return instance._initFromAsset(encodedKey, callback);
     }).then((int length) => instance.._length = length);
   }
 

testing/dart/assets_test.dart

@@ -27,6 +27,13 @@ void main() {
     expect(buffer.length == 354679, true);
   });
 
+  test('Can load an asset with a space in the key', () async {
+    // This assets actual path is "fixtures/DashInNooglerHat%20WithSpace.jpg"
+    final ImmutableBuffer buffer = await ImmutableBuffer.fromAsset('DashInNooglerHat WithSpace.jpg');
+
+    expect(buffer.length == 354679, true);
+  });
+
   test('can dispose immutable buffer', () async {
     final ImmutableBuffer buffer = await ImmutableBuffer.fromAsset('DashInNooglerHat.jpg');
 

testing/dart/fragment_shader_test.dart

@@ -24,6 +24,16 @@ void main() async {
     _expectShaderRendersGreen(shader);
   });
 
+  test('simple shader with space in key renders correctly', () async {
+    final FragmentProgram program = await FragmentProgram.fromAsset(
+      'functions with_space.frag.iplr',
+    );
+    final Shader shader = program.shader(
+      floatUniforms: Float32List.fromList(<double>[1]),
+    );
+    _expectShaderRendersGreen(shader);
+  });
+
   test('blue-green image renders green', () async {
     final FragmentProgram program = await FragmentProgram.fromAsset(
       'blue_green_sampler.frag.iplr',