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.

Sign up for GitHub

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

Jump to bottom

[WGSL] Record type errors while type checking #9603

Closed
wants to merge 0 commits into from
Closed

[WGSL] Record type errors while type checking #9603

wants to merge 0 commits into from

Conversation

tadeuzagallo
Copy link
Member

@tadeuzagallo tadeuzagallo commented Feb 3, 2023 •
edited by webkit-early-warning-system

cd24b94

[WGSL] Record type errors while type checking
https://bugs.webkit.org/show_bug.cgi?id=251686
rdar://105004579

Reviewed by Myles C. Maxfield.

Add a function to start reporting type errors. For now, the errors are only
logged, but they should be reported when we move the type checking phase to
the `staticCheck` stage. To ensure we can report multiple errors, once we
encounter a type error, we infer the type of that expression to be "bottom",
which is a special type that is a subtype of every other type. i.e. we stop
reporting other errors that would be a consequence of an error we already
discovered.

* Source/WebGPU/WGSL/CompilationMessage.cpp:
(WGSL::CompilationMessage::dump const):
* Source/WebGPU/WGSL/TypeCheck.cpp:
(WGSL::TypeChecker::TypeChecker):
(WGSL::TypeChecker::check):
(WGSL::TypeChecker::visit):
(WGSL::TypeChecker::unify):
(WGSL::TypeChecker::isBottom const):
(WGSL::TypeChecker::typeError):
* Source/WebGPU/WGSL/Types.cpp:
(WGSL::printInternal):
(WGSL::toString):
* Source/WebGPU/WGSL/Types.h:

Canonical link: https://commits.webkit.org/259892@main

23bb8a7

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1   🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
❌ 🛠 🧪 merge ✅ 🛠 watch-sim

@tadeuzagallo tadeuzagallo self-assigned this Feb 3, 2023
@tadeuzagallo tadeuzagallo added the WebGPU For bugs in WebGPU label Feb 3, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 1535244)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim

@mwyrzykowski
Copy link
Contributor

Change looks good to me, I will defer to @litherum or @grorg for approval

litherum
litherum approved these changes Feb 3, 2023
View reviewed changes
Source/WebGPU/WGSL/CompilationMessage.cpp Outdated
out.print(m_span.m_line, ":", m_span.m_lineOffset, " length:", m_span.m_length, m_message);
out.print(m_span.m_line, ":", m_span.m_lineOffset, ":", m_message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came to this function to fix the mix colon, but then I didn't think the length was helpful, and it's a pretty standard error format to report only line and column. Happy to add it back though if you think it's helpful.

Source/WebGPU/WGSL/TypeCheck.cpp Outdated
{
// FIXME: Implement all the rules and report a type error otherwise
if (shouldDumpInferredTypes)
dataLogLn("[unify] '", *lhs, "' <", RawPointer(lhs), "> and '", *rhs, "' <", RawPointer(rhs), ">");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WTFLogAlways() :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replied in another PR asking whether you felt strongly about using WTFLogAlways over dataLog, as I don't see why one is strictly better than other, but I don't think you replied. I searched for when to use one over the other, and as far as I could tell it seemed to be a preference thing. dataLog does seem more flexible than WTFLogAlways, so I'd personally prefer to stick to that if you are not strictly opposed.

Source/WebGPU/WGSL/TypeCheck.cpp Outdated

if (shouldDumpInferredTypes) {
for (auto& error : m_errors)
dataLogLn(error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WTFLogAlways() :(

Source/WebGPU/WGSL/TypeCheck.cpp Outdated
@@ -79,12 +85,14 @@ class TypeChecker : public AST::Visitor, public ContextProvider<Type*> {
Type* m_u32;
Type* m_f32;
FixedVector<TypeConstructor> m_typeConstrutors;
WTF::Vector<Error> m_errors;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of think Error and m_errors are bad names, because warnings go here too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error and Warning are aliases to the same underlying class (CompilationMessage). I think we should probably keep them separate, no? It would be helpful to know whether there were any errors or not, although that would mess up the order of reporting... either way, we're only reporting errors for now, I can rename it once we get to warnings.

@tadeuzagallo tadeuzagallo force-pushed the eng/WGSL-Record-type-errors-while-type-checking branch from 1535244 to 4bbac10 Compare February 6, 2023 12:57
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 4bbac10)

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2   🧪 api-mac   🧪 gtk-wk2
✅ 🧪 api-ios   🧪 mac-wk1   🧪 api-gtk
✅ 🛠 tv   🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch   🧪 mac-wk2-stress
✅ 🛠 watch-sim

@tadeuzagallo tadeuzagallo force-pushed the eng/WGSL-Record-type-errors-while-type-checking branch from 4bbac10 to 23bb8a7 Compare February 6, 2023 13:50
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 23bb8a7)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1   🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
❌ 🛠 🧪 merge ✅ 🛠 watch-sim

@tadeuzagallo tadeuzagallo added the merge-queue Applied to send a pull request to merge-queue label Feb 6, 2023
@webkit-commit-queue
Copy link
Collaborator

Committed 259892@main (cd24b94): https://commits.webkit.org/259892@main

Reviewed commits have been landed. Closing PR #9603 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 6, 2023
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/WGSL-Record-type-errors-while-type-checking branch from 23bb8a7 to cd24b94 Compare February 6, 2023 15:34
@tadeuzagallo tadeuzagallo deleted the eng/WGSL-Record-type-errors-while-type-checking branch February 6, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@litherum litherum litherum approved these changes

@mwyrzykowski mwyrzykowski Awaiting requested review from mwyrzykowski

@djg djg Awaiting requested review from djg

@grorg grorg Awaiting requested review from grorg

Assignees

@tadeuzagallo tadeuzagallo

Labels
WebGPU For bugs in WebGPU
Projects
None yet
Milestone
No milestone
5 participants
@tadeuzagallo @webkit-early-warning-system @mwyrzykowski @webkit-commit-queue @litherum