Fix crash while parsing malformed manifest files and make errors prettier #1219

BillyONeal · 2023-09-29T02:51:47Z

Alternative to #1211 ; this version preserves the JSON deserializer invariant that the deserialized type generally can't have semantic constraints. The intended use is that the results are only semantically valid if there were no logged errors.

Fixes microsoft/vcpkg#33973

I highly recommend looking at src/vcpkg-test/manifests.cpp first as most of these changes are nitpick fixes across parsers to get the output to look like.

Alternative to microsoft#1211 Fixes microsoft/vcpkg#33973 I'm not entirely happy with this because it emits extra 'mismatched type' warnings like $.dependencies[0].features[0]: mismatched type: expected a feature in a dependency .

… errors.

BillyONeal · 2023-09-29T02:52:50Z

include/vcpkg/base/messages.h

@@ -84,27 +84,42 @@ namespace vcpkg
        static LocalizedString from_raw(std::basic_string<T>&& s) noexcept;
        static LocalizedString from_raw(StringView s);

-        LocalizedString& append_raw(char c);
-        LocalizedString& append_raw(StringView s);
+        LocalizedString& append_raw(char c) &;


I just stole this out of #1210 :)

BillyONeal · 2023-09-29T02:53:51Z

src/vcpkg-test/manifests.cpp

+                                             PrintErrors::No);
+    REQUIRE(!m_pgh.has_value());
+    REQUIRE(m_pgh.error()->to_string() == "error: while loading <test manifest>:\n"
+                                          "$.default-features[0].name (a default feature): the feature \"core\" turns "


If an error doesn't contain more than a sentence I left everything lowercase with no period.

BillyONeal · 2023-09-29T02:54:02Z

src/vcpkg-test/manifests.cpp

+                                             PrintErrors::No);
+    REQUIRE(!m_pgh.has_value());
+    REQUIRE(m_pgh.error()->to_string() == "error: while loading <test manifest>:\n"
+                                          "$.default-features[0] (a feature name): \"\" is not a valid feature name. "


If an error contains multiple sentences, I ended the first with a period and capitalize to start the next one.

ras0219-msft · 2023-09-29T17:54:00Z

include/vcpkg/base/message-data.inc.h

@@ -994,6 +1007,19 @@ DECLARE_MESSAGE(DeleteVcpkgConfigFromManifest,
                (msg::path),
                "",
                "-- Or remove \"vcpkg-configuration\" from the manifest file {path}.")
+DECLARE_MESSAGE(DependencyFeatureCore,
+                (),
+                "The word \"core\" is an on-disk name that must not be localized. The 'default-features' part is JSON "


Suggested change

"The word \"core\" is an on-disk name that must not be localized. The 'default-features' part is JSON "

"The word \"core\" is an on-disk name that must not be localized. The \"default-features\" part is JSON "

Or they should both use ', but it should be made consistent either way.

Heh I didn't fix all the localizer notes

ras0219-msft · 2023-09-29T17:59:48Z

src/vcpkg-test/dependencies.cpp

@@ -1619,7 +1619,7 @@ TEST_CASE ("version install default features", "[versionplan]")

    auto a_x = make_fpgh("x");
    auto& a_scf = vp.emplace("a", {"1", 0}, VersionScheme::Relaxed).source_control_file;
-    a_scf->core_paragraph->default_features.emplace_back("x");
+    a_scf->core_paragraph->default_features.push_back(DependencyRequestedFeature{"x"});


Suggested change

a_scf->core_paragraph->default_features.push_back(DependencyRequestedFeature{"x"});

a_scf->core_paragraph->default_features.push_back({"x"});

This seems verbose

ras0219-msft · 2023-09-29T18:06:36Z

src/vcpkg-test/registries.cpp

    CHECK(errors[1] ==
          "$.registries[0].packages[2] (a package pattern): \"a*a\" is not a valid package pattern. Package patterns "
          "must use only one wildcard character (*) and it must be the last character in the pattern (see "
-          "https://learn.microsoft.com/vcpkg/users/registries for more information)");
+          "https://learn.microsoft.com/vcpkg/users/registries for more information).");
    CHECK(errors[2] ==
          "$.registries[0].packages[3] (a package pattern): \"*a\" is not a valid package pattern. Package patterns "


(future work) It would be great to break up these extremely long lines into something like:

/in/a/file:10:5: error: "*a" is not a valid package pattern. info: Package patterns must use only one wildcard character (*) and it must be the last character in the pattern. see-also: https://learn.microsoft.com/vcpkg/users/registries

ras0219-msft · 2023-09-29T19:08:30Z

src/vcpkg/sourceparagraph.cpp

+            return DefaultFeatureNameDeserializer::instance.visit_string(r, sv).map(
+                [](std::string&& name) { return DependencyRequestedFeature{std::move(name)}; });


Suggested change

return DefaultFeatureNameDeserializer::instance.visit_string(r, sv).map(

[](std::string&& name) { return DependencyRequestedFeature{std::move(name)}; });

return DefaultFeatureNameDeserializer::instance.visit_string(r, sv);

& make OptionalStorage(Optional<U>) use new (&m_t) T{std::move(*p)};

I really don't want to do that because it will accidentally engage initializer_list ctors :/

…endency-features

Pospelove · 2023-10-02T13:07:59Z

Thank you!

BillyONeal added 3 commits September 28, 2023 00:04

Fix crash while parsing malformed manifest files.

68122ce

Alternative to microsoft#1211 Fixes microsoft/vcpkg#33973 I'm not entirely happy with this because it emits extra 'mismatched type' warnings like $.dependencies[0].features[0]: mismatched type: expected a feature in a dependency .

Remove semantic constraints on the deserialized type and add specific…

b04a5b2

… errors.

Make the e2e tests into unit tests and make the errors a bit prettier.

80fa913

BillyONeal commented Sep 29, 2023

View reviewed changes

Burninate Expr::Empty

918269b

BillyONeal mentioned this pull request Sep 29, 2023

Fix crash while parsing malformed manifest files. #1211

Closed

vicroms approved these changes Sep 29, 2023

View reviewed changes

JavierMatosD approved these changes Sep 29, 2023

View reviewed changes

ras0219-msft approved these changes Sep 29, 2023

View reviewed changes

BillyONeal added 2 commits September 29, 2023 18:57

Merge remote-tracking branch 'origin/main' into add-error-for-bad-dep…

7f9ac40

…endency-features

Code review feedback

582382f

BillyONeal mentioned this pull request Sep 30, 2023

Teach x-ci-verify-versions to check that versions exist in the database #1210

Merged

autoantwort approved these changes Oct 1, 2023

View reviewed changes

BillyONeal merged commit 77d1cc5 into microsoft:main Oct 2, 2023
5 checks passed

BillyONeal deleted the add-error-for-bad-dependency-features branch October 2, 2023 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash while parsing malformed manifest files and make errors prettier #1219

Fix crash while parsing malformed manifest files and make errors prettier #1219

BillyONeal commented Sep 29, 2023

BillyONeal Sep 29, 2023

BillyONeal Sep 29, 2023

BillyONeal Sep 29, 2023

ras0219-msft Sep 29, 2023

ras0219-msft Sep 29, 2023

BillyONeal Sep 30, 2023

ras0219-msft Sep 29, 2023

ras0219-msft Sep 29, 2023

ras0219-msft Sep 29, 2023

BillyONeal Sep 30, 2023

Pospelove commented Oct 2, 2023

	"The word \"core\" is an on-disk name that must not be localized. The 'default-features' part is JSON "
	"The word \"core\" is an on-disk name that must not be localized. The \"default-features\" part is JSON "

	a_scf->core_paragraph->default_features.push_back(DependencyRequestedFeature{"x"});
	a_scf->core_paragraph->default_features.push_back({"x"});

		return DefaultFeatureNameDeserializer::instance.visit_string(r, sv).map(
		[](std::string&& name) { return DependencyRequestedFeature{std::move(name)}; });

Fix crash while parsing malformed manifest files and make errors prettier #1219

Fix crash while parsing malformed manifest files and make errors prettier #1219

Conversation

BillyONeal commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pospelove commented Oct 2, 2023