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

[Lang] Fix ti.static(ti.grouped(ti.ndrange(...))) syntax checker false positive #680

Merged
merged 2 commits into from
Mar 30, 2020

Conversation

yuanming-hu
Copy link
Member

This is a special case that we should let pass. A more systematic solution is to defer the checking to the stage when the AST generating script is executed.

Related issue = #taichi-dev/taichi_elements#13

[Click here for the format server]

@yuanming-hu
Copy link
Member Author

yuanming-hu commented Mar 29, 2020

@xumingkuan Sorry about the brute-force treatment here. No rush to review it during the weekend.

Forgive me if I bypass your review - I'm just trying to have this in v0.5.10 releasing tonight so that people won't get a syntax error. We can have more discussions for a more systematic solution after this urgent issue is resolved.

Copy link
Collaborator

@xumingkuan xumingkuan left a comment

Choose a reason for hiding this comment

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

Oh yes, I wrote this after #631 (comment) to raise a loud error to avoid silent errors as I didn't check if it would work. Let's merge this in swiftly and make it work today.

@yuanming-hu
Copy link
Member Author

yuanming-hu commented Mar 29, 2020

I see. Thanks for understanding. I didn't realize this use case could be an issue during code review either. I'll merge this in once CI passes.

@xumingkuan
Copy link
Collaborator

Did it work in previous versions? I don't think I changed the behavior of ti.static for loops...

@yuanming-hu
Copy link
Member Author

Did it work in previous versions? I don't think I changed the behavior of ti.static for loops...

You mean the test I skipped? It's now throwing an AssertionError instead of TaichiSyntaxError after this PR. That's why I skip it.

@xumingkuan
Copy link
Collaborator

Oh I mean did ti.static(ti.grouped(x)) work correctly in previous versions. I'm wondering why it gives AssertionError now.

@yuanming-hu
Copy link
Member Author

Oh I mean did ti.static(ti.grouped(x)) work correctly in previous versions.

It doesn't work should not work. Taichi will throw an AssertionError before your refactoring, which is not as informative as your TaichiSyntaxError. I just changed it back to the old behavior.

I'm just letting ti.static(ti.groupd(ti.ndrange())) pass here.

@xumingkuan
Copy link
Collaborator

@yuanming-hu Shall we have a Skype chat about how to fix this now?

@yuanming-hu
Copy link
Member Author

Sure!

@yuanming-hu yuanming-hu merged commit ef48817 into taichi-dev:master Mar 30, 2020
@yuanming-hu yuanming-hu deleted the fix-syntax branch March 30, 2020 00:05
@xumingkuan
Copy link
Collaborator

How to raise TaichiSyntaxError in the emitted code of transformer.py? It looks like ti.TaichiSyntaxError is defined in transformer.py and I cannot use it in the emitted code.

@yuanming-hu
Copy link
Member Author

Oh please move TaichiSyntaxError outside and export it, so that you can use ti.TaichiSyntaxError.

@xumingkuan
Copy link
Collaborator

Which file should TaichiSyntaxError belong to?

@yuanming-hu
Copy link
Member Author

Which file should TaichiSyntaxError belong to?

Good question - let's create taichi/python/taichi/lang/exception.py to hold these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants