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

Fix scope reference of internal definition and macro expansion #328

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@Hamayama
Contributor

Hamayama commented Dec 11, 2017

内部 define とマクロ展開の組み合わせで、
外側のスコープを参照してしまう件について調べました。

ちょっと影響範囲が読めない感じですが、まずは情報としてあげておきます。

#327 の (2)-(a)(b)(c) については、本パッチでOKになることを確認しました。

<原因>
pass1/body-rec の処理で、内部 define 用の frame (vframe) が作られて、
内部 define の変数は、そこに順番に追加されていく。

しかし、マクロ展開時に呼ばれる Scm_MakeIdentifier では、ヒットしない frame を
除外して identifier の env を記憶するようになっている。

このため、変数が vframe に登録される前にマクロ展開が行われると、
マクロ展開結果の identifier には vframe を除外した env が記憶されてしまい、
後から vframe に変数が追加されても、identifier からは参照できない。

結果として、identifier が外側のスコープを参照してしまう。

<対策>
Scm_MakeIdentifier で、frame の除外を行わないようにする。

<気になる点>
(1) frame を除外しないことで、identifier 検索時の負荷が増えると思われる。

(2) identifier の env が「ヒットする frame だけを持つこと」を当てにしている
プログラムがあると、不具合が発生すると思われる。
→ 実際に precomp.scm で 以下のエラー
「identifier with compiler environment can't be compiled」
→ identifier-unbound? を追加して対応した
→ あと、Scm_IdentifierBindingEqv も、まずそうだったので変更

<テスト結果>
OS : Windows 8.1 (64bit)
Gauche : コミット bb58efd + 変更
開発環境 : MSYS2/MinGW-w64 (64bit) (gcc version 7.1.0 (Rev2, Built by MSYS2 project))
Total: 19268 tests, 19268 passed, 0 failed, 0 aborted.

@shirok

This comment has been minimized.

Owner

shirok commented Dec 12, 2017

ああなるほど! identifierの同定が早過ぎるところまでは突き止めてて、どうやって遅らせようかを考えていたんですが、envをいじるという方法がありましたね。
一度identifierを同定してしまえば後は変わらないので、性能問題はget_binding_frameの結果をキャッシュすれば良いと思います。
他、若干修正してパッチ頂きます。

shirok added a commit that referenced this pull request Dec 12, 2017

Fix scope error with internal defines
When an identifier is inserted by the macro expansion that supposed to
refer to the identifier inserted in the same level with an internal define,
but such internal define appears _later_ than macro expansion, we failed
to recognize it.
This fix is based on @Hamayam's PR #328
@shirok

This comment has been minimized.

Owner

shirok commented Dec 12, 2017

こちらでfixしました。36c7279

@Hamayama

This comment has been minimized.

Contributor

Hamayama commented Dec 12, 2017

こちらの環境でも問題ないことを確認しました。
いろいろとありがとうございました。

@Hamayama Hamayama closed this Dec 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment