Add support for optional attributes on HTML elements/tags

[ilyvion]

Description

I've added support for optional attributes on HTML elements/tags as described in #903. That is, this is now possible:

let url1 = Some("https://example.com");

html! {
    <div>
        <a href?=url1 media?=Some("print")>{ "Some URL" }</a>
    </div>
}

Fixes #903

Checklist:

• I have run ./ci/run_stable_checks.sh (Can't, I'm on Windows, but I did the things manually.)
• I have reviewed my own code
• I have added tests
• I have opened a PR against https://github.com/yewstack/js-framework-benchmark: Bench alexschrod/yew#issue-903 js-framework-benchmark#9

[siku2]

I have a few issues with the current implementation (I hope my comments don't come across as too negative) but I'm generally in support of this feature.

[siku2]

Oh and just to make sure we're not wasting our time here.
@jstarry, do you agree that this feature is worth adding?

[jstarry]

Oh and just to make sure we're not wasting our time here.
@jstarry, do you agree that this feature is worth adding?

I agree that it's a nice feature to have. Looks like the current implementation will generate extra code every time the special syntax is used. Maybe we could implement this in a different way with Into<Option<T>> arg types for VTag setters

[siku2]

@alexschrod if you don't mind, could you also add some (failing) compilation tests for the other tag-like constructs like components and lists (<>{ "fragments" }</>).
This is just to make sure we have nice error messages for those cases.

[ilyvion]

@alexschrod if you don't mind, could you also add some (failing) compilation tests for the other tag-like constructs like components and lists (<>{ "fragments" }</>).
This is just to make sure we have nice error messages for those cases.

Do you mean doing so in this PR? Feels a bit out of scope, although I'm not opposed to doing this in a separate PR if you would like me to.

[siku2]

@alexschrod

Do you mean doing so in this PR? Feels a bit out of scope, although I'm not opposed to doing this in a separate PR if you would like me to.

I'm only talking about the new syntax. For instance, you're missing a test for the error message emitted when trying to use the syntax on components.
I fail to see how this is out of scope?

[ilyvion]

I'm only talking about the new syntax. For instance, you're missing a test for the error message emitted when trying to use the syntax on components.
I fail to see how this is out of scope?

I misunderstood what you meant. I get it now. Will do!

[ilyvion]

@alexschrod if you don't mind, could you also add some (failing) compilation tests for the other tag-like constructs like components and lists (<>{ "fragments" }</>).

I've added a failing compilation test for components (not committed/pushed yet). I'm not sure I understand what you mean by "lists" and how would you even add an attribute to a fragment?

[siku2]

@alexschrod
Originally I wanted to test the error message for <some_attr=None></> but you're right, that doesn't really make sense...
However, lists would be a great place to test the error for the special key attribute:

<key?=None>
  <div>{ "Hello World!" }</div>
</>

This should cause the error "The 'key' attribute does not support being used as an optional attribute".

[ilyvion]

@siku2 Currently prevented from doing so by #1444

[siku2]

It might look like a lot but most of my comments are tiny things.

There is however still a rather big issue that I'd like to tackle. Currently even non-optional attributes are wrapped in Option::Some. Even if the compiler is able to completely optimize this away (which would need to be tested) it would come at the cost of increased compile time.
It should definitely be possible to refactor the code so that normal attributes aren't wrapped in Option. One way to achieve this might be to add them to the vtag directly instead of storing them in attr_pairs first.
The approach suggested by @jstarry would also do the trick because Into<Option<T>> is implemented for all T as Some(T).

[ilyvion]

I don't know why this is the case, but the build case fails because of this difference in output:

--- alexschrod
+++ Travis
@@ -301,13 +301,8 @@
 error[E0277]: `NotToString` doesn't implement `std::fmt::Display`
     --> $DIR/html-tag-fail.rs:49:23
      |
-49   |     html! { <a media?=Some(NotToString) /> };
-     |                       ^^^^ `NotToString` cannot be formatted with the default formatter
-     |
-    ::: $RUST/src/liballoc/string.rs:2194:1
-     |
-2194 | pub trait ToString {
-     | ------------------ required by this bound in `std::string::ToString::to_string`
+49   |       html! { <a media?=Some(NotToString) /> };
+     |                         ^^^^ `NotToString` cannot be formatted with the default formatter
      |
      = help: the trait `std::fmt::Display` is not implemented for `NotToString`
      = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
@@ -344,13 +339,8 @@
 error[E0277]: `NotToString` doesn't implement `std::fmt::Display`
     --> $DIR/html-tag-fail.rs:54:24
      |
-54   |     html! { <li value?=Some(NotToString) /> };
-     |                        ^^^^ `NotToString` cannot be formatted with the default formatter
-     |
-    ::: $RUST/src/liballoc/string.rs:2194:1
-     |
-2194 | pub trait ToString {
-     | ------------------ required by this bound in `std::string::ToString::to_string`
+54   |       html! { <li value?=Some(NotToString) /> };
+     |                          ^^^^ `NotToString` cannot be formatted with the default formatter
      |
      = help: the trait `std::fmt::Display` is not implemented for `NotToString`
      = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead

I can't help it that my rustc decides to throw in something about $RUST/src/liballoc/string.rs... It's not like I have a weird Rust version or anything.

> rustc --version
rustc 1.45.0 (5c1f21c3b 2020-07-13)

[ilyvion]

Travis appears to be having issues.

[siku2]

Travis should be working again. Just need to update to the latest commit.

[siku2]

@alexschrod There have been some significant changes to the macro internals. I updated the code and opened a new PR: #1537
Thanks for all the work you have done here!

[siku2]

#1537 just landed 🎉