Fix TypeUse #23

kariya-mitsuru · 2020-05-31T14:50:45Z

Inline parameter and result declaration without typeidx should insert
type definition at the last of module.

Inline parameter and result declaration with typeidx should check
consistency.

Passed new 10 tests.

before

End ".../wain/spec-test/wasm-testsuite/func.wast":
  total: 130, passed: 124, failed: 6, skipped: 0

End ".../wain/spec-test/wasm-testsuite/call_indirect.wast":
  total: 153, passed: 149, failed: 4, skipped: 0

Results of 76 files:
  total: 19757, passed: 19416, failed: 212, skipped: 129

after

End ".../wain/spec-test/wasm-testsuite/func.wast":
  total: 130, passed: 130, failed: 0, skipped: 0

End ".../wain/spec-test/wasm-testsuite/call_indirect.wast":
  total: 153, passed: 153, failed: 0, skipped: 0

Results of 76 files:
  total: 19757, passed: 19426, failed: 202, skipped: 129

Inline parameter and result declaration without typeidx should insert type definition at the last of module. Inline parameter and result declaration with typeidx should check consistency.

rhysd · 2020-05-31T17:14:10Z

wain-syntax-text/src/wat2wasm.rs

+                .iter()
+                .map(|wat::TypeDef { ty, .. }| ty.clone())
+                .collect(),
+        );


ctx.types.reserve(self.types.len()); for type_def in self.types.iter() { ctx.types.push(type_def.ty.clone()); }

にしてもらえると，.collect() によるアロケーションが不要になりそうです

rhysd · 2020-05-31T17:14:43Z

wain-syntax-text/src/parser.rs

+            results: results.to_vec(),
+        });
+        idx as u32
+    }


メソッド定義の前後は1行空白をあけておいていただけますでしょうか

rhysd · 2020-05-31T17:20:23Z

wain-syntax-text/src/parser.rs

-            idx: Index::Num(idx),
+            idx: match idx {
+                Some(idx) => RefOrInline::Reference(idx),
+                None => RefOrInline::Inline(parser.create_inline_typeuse(start, &params, &results)),


これって inline というワードの使い方適切でしょうか？暗黙に自動で生成される ID は仕様では fresh id と呼ばれていたので，fresh のほうが良さそうに感じました

inline は spec で使用されている言葉です。

A typeuse may also be replaced entirely by inline parameter and result declarations.

spec 上この構文要素（typeuse）では fresh id と言う用語は出てきません。

A typeuse may also be replaced entirely by inline parameter and result declarations.

https://www.w3.org/TR/2019/REC-wasm-core-1-20191205/#abbreviations%E2%91%A6

この inline は parameter and result declarations にかかっているので，例えば (type 0) (param) の代わりに param が直接書かれているのを指して inline と書いてるのではないでしょうか？typeuse の index を省略した時を指して inline と書いているようには読めませんでした．

spec 上この構文要素（typeuse） fresh id と言う用語は出てきません。

すみません，これは僕の勘違いでした．fresh identifier は function, memory, table, global だけですね．

この inline は parameter and result declarations にかかっているので，例えば (type 0) (param) の代わりに param が直接書かれているのを指して inline と書いてるのではないでしょうか？typeuse の index を省略した時を指して inline と書いているようには読めませんでした．

それはその通りです。
が、"(type n)" と "(type n) (param)* (result)*" のパターンと "(param)* (result)*" のパターンを区別する短くて良い単語が他に思い浮かばなかったので、型定義への参照を含むパターンを Reference（param と result は外側の TypeUse 構造体に含まれているのでそれ自身で有無を区別する必要が無い）、型定義への参照を含まずインライン宣言のみのパターンを Inline 、と言う名称にしました。

とは言え、列挙型名の RefOrInline も含めてあまり良い名前とは思っていないので（あまり？）、何か良い命名があればさっくり直します。

下記に合わせて Implicit というのはどうでしょう？また enum の名前は僕も良いものが思い付かないのですが，明示するものと暗黙に挿入されるものの2種類があるという意味で TypeUseKind とかどうでしょう？

enum TypeUseKind<'s> { Explicit(Index<'s>), Implicit(u32), }

rhysd · 2020-05-31T17:23:53Z

wain-syntax-text/src/wat2wasm.rs

-                .results
-                .iter()
-                .map(|p| p.ty.transform(ctx))
-                .collect::<Result<'_, _>>()?,


問題ないのですが，ここってどうして params と results を外に移したのでしょう？borrow checker がらみでしょうか？

あ、すみません、ボロ―チェッカやデータの値確認とかで悪戦苦闘してた際に切り出したりしてたんですが、最終的に元の状態で上手くいくので戻します。

rhysd · 2020-05-31T17:30:05Z

wain-syntax-text/src/wat2wasm.rs

@@ -470,7 +518,7 @@ impl<'s> Transform<'s> for wat::Instruction<'s> {
            wat::InsnKind::Return => wasm::InsnKind::Return,
            wat::InsnKind::Call(idx) => wasm::InsnKind::Call(ctx.resolve_func_idx(idx, start)?),
            wat::InsnKind::CallIndirect(ty) => {
-                wasm::InsnKind::CallIndirect(ctx.resolve_type_idx(ty.idx, start)?)
+                wasm::InsnKind::CallIndirect(ctx.resolve_type_idx(&ty, start)?)


ここムーブして渡していただけますでしょうか？そうすれば resolve_type_idx は参照で受け取る必要がなく，したがって resolve_index も Index を参照で受け取る必要がなくなりそうです

rhysd · 2020-05-31T17:33:10Z

wain-syntax-text/src/wat2wasm.rs

@@ -265,6 +302,8 @@ pub fn wat2wasm<'s>(
        local_indices: Indices::new(),
        next_local_idx: 0,
        label_stack: LabelStack::new(source),
+        tentatives: std::mem::replace(&mut parsed.module.tentatives, Vec::new()),


Suggested change

tentatives: std::mem::replace(&mut parsed.module.tentatives, Vec::new()),

tentatives: std::mem::take(&mut parsed.module.tentatives),

意味は同じですが，take でよりシンプルになりそうです

rhysd · 2020-05-31T17:34:14Z

wain-syntax-text/src/parser.rs

@@ -248,6 +248,7 @@ struct ParseContext<'s> {
    table_indices: Indices<'s>,
    mem_indices: Indices<'s>,
    global_indices: Indices<'s>,
+    tentatives: Vec<TentativeTypeUse<'s>>,


tentative は temp と同じぐらい情報量が無いので，fresh_types または implicit_types とかでお願いできませんでしょうか？複数のソース（parser.rs と wat2wasm.rs）でまたがって使う値なので，なるべく説明的な変数名を付けておきたいです

用語 fresh は fresh id ではないのでミスリード、用語 implicit も、この時点では暗黙定義するとは限らない（単なる参照の場合がある）ので不適切と考えました。
ちなみに、tentative は C 言語では仮定義の意味で tentative definition と言うように使用します。今回のケースでは、C 言語では仮定義と同様、定義になる場合と単なる参照になる場合の両方があるためニュアンス的に合ってるなと思い、この用語を使用しました。

「何の？」に当たる部分が無いのはご指摘通りなので、tentative_types ではどうでしょうか？（「仮の」のニュアンスが欲しいな、と思いまして。ホントは tentative_type_definitions とかのほうがいいのかもしれませんが、流石に長すぎる気が…）
他に良い用語があればそれでも構いませんし、もちろんやはり fresh か implicit が良ければそちらにします。

~~完全に好みの問題で申し訳ないですが，unresolved_type_uses が良いです！！~~

すみません，誤読してましたが，tentatives は自動挿入される type use の添字 → type use の index のマッピングを表すものなんですね．wat のパースが終わった時点で resolve 自体は完了しているという．うーむ

inserted_type_to_index とかになるのかな… 一周回って kariya さん案の tentative_types のほうが簡潔で良い気もしてきました…（将来的にもっと良い名前を思い付いたらリファクタする感じで…）

暗黙に指定されているという意味で implicit_types または implicit_type_uses が良いように思います

メンバ名は implicit_type_uses、構造体名も ImplicitTypeUse で修正しました。

rhysd · 2020-05-31T17:45:19Z

ありがとうございます．

ちなみにこの fresh ID を後で解決しないといけないのって，type use に限らず table とか memory もそうですよね…

rhysd · 2020-06-01T01:56:13Z

wain-syntax-text/src/wat2wasm.rs

+                if ty.params.is_empty() && ty.results.is_empty()
+                    || ty.params.len() == params.len()
+                        && ty.results.len() == results.len()
+                        && ty
+                            .params
+                            .iter()
+                            .zip(params.iter())
+                            .all(|(l, r)| l.ty == r.ty)
+                        && ty
+                            .results
+                            .iter()
+                            .zip(results.iter())
+                            .all(|(l, r)| l.ty == r.ty)


Suggested change

if ty.params.is_empty() && ty.results.is_empty()

|| ty.params.len() == params.len()

&& ty.results.len() == results.len()

&& ty

.params

.iter()

.zip(params.iter())

.all(|(l, r)| l.ty == r.ty)

&& ty

.results

.iter()

.zip(results.iter())

.all(|(l, r)| l.ty == r.ty)

if ty.params

.iter()

.map(|p| p.ty)

.eq(params.iter().map(|p| p.ty))

&& ty

.results

.iter()

.map(|r| r.ty)

.eq(results.iter().map(|r| r.ty))

.eq() を使うとシーケンスの長さをチェックする必要がなくなるので，よりシンプルになりそうです．

ちなみに、is_empty() は省略できないので残しました。

あっ，すみません，そうですね．僕の suggestion のほうが間違いです

kariya-mitsuru · 2020-06-01T05:13:18Z

ちなみにこの fresh ID を後で解決しないといけないのって，type use に限らず table とか memory もそうですよね…

上のコメントにもリプライしましたが、typeuse では fresh ID は出てきません。
逆に、fresh ID の解決は（まだちゃんと確認してませんが）後である必要が無いと思います。

typeuse は以下の点で特殊だと思います。

型定義を自動挿入すべきかどうかはモジュールを最後まで見ないとわからない。
型定義を自動挿入する必要がある場合は、モジュールの最後に挿入する必要がある。

まぁ2点目は1点目の裏返しですが…

Use reserve & push instead of map & collect to avoid unnecessary allocation. Add blank line arround the create_inline_typeuse method definition. Revert unnecessary local variable introduction. Change the parameter type of the resplve_type_idx from reference to non reference. Change std::mem::replace to std::mem::take for simplicity.

Use more better names for struct & enum.

rhysd

💯

rhysd · 2020-06-01T16:44:34Z

いつもありがとうございます！

Fix TypeUse

67ece75

Inline parameter and result declaration without typeidx should insert type definition at the last of module. Inline parameter and result declaration with typeidx should check consistency.

kariya-mitsuru force-pushed the fix-typeuse branch from 2a00a22 to 67ece75 Compare May 31, 2020 15:26

rhysd requested changes May 31, 2020

View reviewed changes

rhysd requested changes Jun 1, 2020

View reviewed changes

kariya-mitsuru added 2 commits June 1, 2020 19:13

Follow Github comments

82ef008

Use more better names for struct & enum.

rhysd approved these changes Jun 1, 2020

View reviewed changes

rhysd merged commit af84260 into rhysd:master Jun 1, 2020

kariya-mitsuru deleted the fix-typeuse branch June 1, 2020 17:02

rhysd added a commit that referenced this pull request Jun 13, 2020

better error message on function type mismatch (follow up for #23)

5e08d37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TypeUse #23

Fix TypeUse #23

kariya-mitsuru commented May 31, 2020

rhysd May 31, 2020 •

edited

rhysd May 31, 2020

rhysd May 31, 2020

kariya-mitsuru Jun 1, 2020

rhysd Jun 1, 2020 •

edited

kariya-mitsuru Jun 1, 2020

rhysd Jun 1, 2020

rhysd May 31, 2020 •

edited

kariya-mitsuru Jun 1, 2020

rhysd May 31, 2020

rhysd May 31, 2020

rhysd May 31, 2020

kariya-mitsuru Jun 1, 2020

rhysd Jun 1, 2020 •

edited

rhysd Jun 1, 2020

rhysd Jun 1, 2020

kariya-mitsuru Jun 1, 2020

rhysd commented May 31, 2020

rhysd Jun 1, 2020

kariya-mitsuru Jun 1, 2020

rhysd Jun 1, 2020

kariya-mitsuru commented Jun 1, 2020

rhysd left a comment

rhysd commented Jun 1, 2020

	tentatives: std::mem::replace(&mut parsed.module.tentatives, Vec::new()),
	tentatives: std::mem::take(&mut parsed.module.tentatives),

Fix TypeUse #23

Fix TypeUse #23

Conversation

kariya-mitsuru commented May 31, 2020

rhysd May 31, 2020 • edited

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

rhysd Jun 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhysd May 31, 2020 • edited

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

rhysd Jun 1, 2020 • edited

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

rhysd commented May 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kariya-mitsuru commented Jun 1, 2020

rhysd left a comment

Choose a reason for hiding this comment

rhysd commented Jun 1, 2020

rhysd May 31, 2020 •

edited

rhysd Jun 1, 2020 •

edited

rhysd May 31, 2020 •

edited

rhysd Jun 1, 2020 •

edited