Skip to content
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

Предупреждение о связанных переменных в образцах #290

Closed
Mazdaywik opened this issue May 25, 2020 · 7 comments · Fixed by #347
Assignees
Labels

Comments

@Mazdaywik
Copy link
Member

Проблема

В присваивании, блоке или вложенной функции легко забыть записать крышку у переопределяемой переменной. Типичный пример — коммит a21d2d9.

В лучшем случае потеря крышки приведёт к снижению быстродействия (т.к. переменная сначала копируется, а потом сравнивается на равенство), в худшем — внесётся ошибка (т.к. значение переменной может быть иным).

Решение

Ну, очевидно нужно отлавливать такие случаи и выдавать на них предупреждение. Причём указывать можно непосредственно на переменную — переменные в дереве у Checker.ref’а имеют координаты.

Засада

Засада в том, что повторная переменная может быть намеренной частью алгоритма. Например:

DoOptTree {
0 t.OptDrive s.OptSpec s.OptAutoMarkup e.AST = 0 e.AST;
s.Cycles t.OptDrive s.OptSpec s.OptAutoMarkup e.AST
= <Log-AST ('Pass ' <Symb s.Cycles> ' (before Auto markup)') e.AST> : e.AST^
= e.AST : e.OriginAST
= <OptTree-AutoMarkup s.OptAutoMarkup e.AST> : e.AST^
= <Dec s.Cycles> : s.Cycles^
= <DriveSpecLoop s.Cycles t.OptDrive s.OptSpec e.AST> : s.Cycles^ e.AST^
= e.AST
: {
e.OriginAST = s.Cycles e.OriginAST;
e.AST^ = <DoOptTree s.Cycles t.OptDrive s.OptSpec s.OptAutoMarkup e.AST>;
}
}

DoDriveSpecLoop {
0 t.OptDrive s.OptSpec e.AST = 0 e.AST;
s.Cycles t.OptDrive s.OptSpec e.AST
= e.AST : e.OriginAST
= <DriveLoop s.Cycles t.OptDrive e.AST> : s.Cycles^ e.AST^
= <SpecPass s.Cycles s.OptSpec e.AST> : s.Cycles^ e.AST^
= e.AST
: {
e.OriginAST = s.Cycles e.OriginAST;
e.AST^ = <DoDriveSpecLoop s.Cycles t.OptDrive s.OptSpec e.AST>;
}
}

DoDriveLoop {
0 t.OptDrive e.AST = 0 e.AST;
s.Cycles t.OptDrive e.AST
= <Log-AST ('Pass ' <Symb s.Cycles> ' (before Drive)') e.AST> : e.AST^
= e.AST : e.OriginAST
= <ExpandClosures e.AST> : e.AST^
= <OptTree-Drive t.OptDrive e.AST> : e.AST^
= <Dec s.Cycles> : s.Cycles^
= e.AST
: {
e.OriginAST = s.Cycles e.OriginAST;
e.AST^ = <DoDriveLoop s.Cycles t.OptDrive e.AST>;
};
}

Все три примера проверяют, что дерево осталось неизменным с предыдущей итерации. Поэтому просто выдавать предупреждения на повторные переменные в образцах блоков и вложенных функций нельзя — будет много ложноположительных срабатываний.

Повторная переменная может быть ключом при ассоциативном поиске (по открытой переменной), а значит, тоже является частью алгоритма.

Наиболее консервативной (но и наиболее слабой) эвристикой можно рассматривать повторные переменные в присваиваниях с жёсткими образцами. Но этого мало. Нужны более тонкие эвристики для блоков и вложенных функций.

@Mazdaywik
Copy link
Member Author

Коммит выше демонстрирует «лучший случай» этой ошибки — потеря быстродействия. Ошибка, к слову, была обнаружена при просмотре профиля.

По хорошему, нужно поискать побольше коммитов на эту ошибку дабы набрать статистику. Можно просто поискать исправления, добавляющие ^.

@Mazdaywik
Copy link
Member Author

Mazdaywik commented May 25, 2020

Полный список коммитов в хронологическом порядке:

  1. 42b5512 (см. выше),
  2. a21d2d9 (см. выше),
  3. dc1765a (но тут исходный файл портировался с классического Рефала-5),
  4. ca96a72 (ошибка вызывала проблемы производительности),
  5. 689a7f9,
  6. 2fa8dd6 (функция IsIdentTail в самом конце диффа),
  7. 9fac618,
  8. 5032ffc,
  9. 857a2d7 (файл Desugaring.sref),
  10. 6b4a0cc,
  11. d63583c (файл src/lexgen/Parser.sref, функция ParseSetDescr).

Поиск осуществлялся в логе, полученном командой:

git log -p --word-diff-regex=[^[:space:]]

Искалась строчка {+^+}, коммиты с найденной строчкой (и не являющиеся случайными ложными вхождениями) перечислены выше.

Проблема имеет место. Теперь нужно проанализировать контекст и делать вывод об эвристиках.

@Mazdaywik Mazdaywik changed the title Предупреждение о повторных переменных Предупреждение о связанных переменных в образцах May 26, 2020
@Mazdaywik
Copy link
Member Author

Mazdaywik commented May 26, 2020

Подготовка «белого списка»

Есть 11 примеров ошибок, которые сразу не выявились и закоммитились. Разумеется, таких ошибок на самом деле было больше, только они выявлялись сразу и исправлялись.

С другой стороны есть целый рабочий компилятор Рефала-5λ, в котором есть присваивания, блоки, вложенные функции и связанные переменные в образцах (хотя не исключены не выявленные ошибки).

Поэтому нужно подобрать эвристики, которые (а) детектят эти 11 случаев, и (б) молча принимают исходный текст компилятора.

На первом этапе исследований нужно будет детектировать все связанные переменные в образцах присваиваний, блоков и вложенных функций, чтобы понять, чем наполнить «белый список».

Подавление предупреждений

Допустим, компилятор выявил связанную переменную в образце, показал на неё программисту. Программист смотрит и говорит: «Спасибо компилятор, я именно это и имел ввиду», т.к. там действительно должна быть повторная переменная (как в случае с синтаксическими деревьями в заглавном посте).

Поэтому нужен способ подавлять предупреждения компилятора для ложноположительных случаев.

Предлагается такой способ — комментарий /* same */, который ставится в той же строчке. Комментарий распознаётся лексическим анализатором, он выдаёт токен-подавитель (TkSuppressor), который помещается в список ошибок. Список ошибок при печати удаляет предупреждение -Wrepeat-variable (так мы его назовём) в тех же номерах строк, что и комментарий-подавитель.

В принципе, можно предусмотреть общую форму подавителей вида /* SUPPRESS: -Wwarning-name */, подавляющую любое предупреждение в данной строке. Но для данного случая удобнее использовать специальный комментарий /* same */ (без учёта регистра и пробельных символов).

Подавление предупреждений вынесено в отдельную задачу #345.

@Mazdaywik Mazdaywik added the study label Jul 3, 2020
@Mazdaywik Mazdaywik removed their assignment Jul 3, 2020
@Mazdaywik Mazdaywik added this to the study fall 2020 milestone Jul 3, 2020
@Mazdaywik Mazdaywik removed the task label Jul 3, 2020
@Mazdaywik
Copy link
Member Author

Неплохая задача для курсовой работы по компиляторам или ВКР.

@Mazdaywik Mazdaywik modified the milestones: study fall 2020, summer 2020 Jul 3, 2020
@Mazdaywik
Copy link
Member Author

А, впрочем, можно предложить её и на летнюю практику.

@Mazdaywik
Copy link
Member Author

Только что написал:

RemoveReferenceTagFromChildren {
  (Func (e.Name) Children (e.Children))
    = <Map
        {
          (INDIRECT) = /* пропускаем */;
          (s._ e.Name) = (e.Name);
        }
        e.Children
      >
    : e.Children^
    = (Func (e.Name) Children (e.Children));
}

И радостно получил ошибку! Потому что переменная e.Name внутри вложенной функции совпадает с e.Name снаружи.

@Mazdaywik
Copy link
Member Author

Ещё была проблема:

    = <Map &MakeGraphNode e.AST> : e.Graph
    = <Map &RemoveReferenceTagFromChildren e.Graph> : e.Graph^

    = <MapAccum &UnsuffixedFunctions () e.Graph> : (e.Roots) e.Graph

В последнем присваивании должно быть e.Graph^. Но это не ошибка, т.к. функция UnsuffixedFuncions граф не меняла — MapAccum использовался как раз для избежания копирования. А вместо этого происходило и копирование, и сравнение на равенство.

penachett added a commit that referenced this issue Mar 5, 2021
Mazdaywik added a commit to Mazdaywik/refal-5-lambda that referenced this issue Mar 9, 2021
Ошибка приводила к падению компилятора во front-end’е Простого Рефала.
Mazdaywik added a commit to Mazdaywik/refal-5-lambda that referenced this issue Mar 9, 2021
На ошибку указало ложное срабатывание диагностики -Wrepeated-maybe (bmstu-iu9#290).
Хоть срабатывание по сути было ложным: равенство повторной переменной требует
сама логика алгоритма, но оно показало на реальную ошибку: вместо проверки
результатного выражения дважды проверялся образец.
Mazdaywik added a commit to Mazdaywik/refal-5-lambda that referenced this issue Mar 9, 2021
Теперь повторная переменная считается подозрительной, если она однозначно
сопоставляется в образце и образец находится в точке, откуда нет отката.

Отката нет в присваиваниях, в образцах последних предложений, в условиях
при присваиваниях и в последних предложениях, если предшествующий
образец сопоставляется однозначно (открытых переменных нет).

Открытые переменные по-прежнему переоцениваются, т.к. проверяются образцы
вида

    … e.X … e.Y …

без проверки того, что эти переменные не повторные.

Теперь -Wrepeated сообщает на подозрительные переменные, -Wrepeated-maybe —
на однозначно сопоставляемые повторные переменные в точках, где откат есть.
Mazdaywik added a commit to Mazdaywik/refal-5-lambda that referenced this issue Mar 9, 2021
Предупреждение -Wrepeated (bmstu-iu9#290) показало на избыточное сравнение с повторной
переменной. Оказалось, эту переменную передавать отдельно в функцию
UnCondition-Sentence нет никакой необходимости.
Mazdaywik added a commit to Mazdaywik/refal-5-lambda that referenced this issue Mar 9, 2021
Эти предупреждения неинформативные и были добавлены лишь в экспериментальных
целях. Эксперимент позволил отладить те случаи, когда предупреждения
наиболее осмысленны — это ситуации, когда выдаётся -Wrepeated.

Для предупреждения -Wrepeated написано более подходящее сообщение.
Mazdaywik added a commit that referenced this issue Jun 26, 2021
Ранее специализация замыкания {{ &F CONTENT }} осуществлялась как
специализация фиктивного вызова <F CONTENT e.@>. Оптимизатор строил новый
вызов <F@1 CONTENT′ e.@>, из которого восстанавливалось замыкание
{{ &F@1 CONTENT′ }}.

Добавлять и удалять фиктивную переменную e.@ было безопасно, т.к. она была
в позиции динамического параметра, позиция параметра не менялась (оставалась
последней) и во внутрь функции она не протекала.

Теперь же при специализации это имя может протечь внутрь экземпляра,
внутри экземпляра может оказаться другое замыкание, в контексте которого
будет переменная e.@. Добавление e.@ в конец вызовет конфликт имён.

----

В коде есть некоторый костыль, который проистекает из отсутствия поддержки
подавления предупреждений. Исходники намеренно собираются с -Werror,
а в данном коде было бы разумно явно добавить «подозрительную» (#290)
повторную переменную. Пришлось достигать эту проверку более громоздким
куском исходного кода.
Mazdaywik added a commit that referenced this issue Jul 21, 2021
Формально, данные повторные переменные не подходят под критерий #290:
однозначное вхождение в последнем предложении. Но в случае второго
предложения здесь:

    : {
      Generalize t.OptInfo^ 1 = …
      Generalize t.OptInfo^ s.Cnt = …
      Tree t.DriveTreeState e.Branches^ = …
    }

можно выдать предупреждение на t.OptInfo по следующему критерию —
последующие предложения не способны поглотить аргумент, совпадающий
с данным. И поэтому переменная подозрительная.

Так что, возможно, критерий стоит уточнить.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants