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

Matchers do not warn on improper use #160

Open
adamkaplan opened this issue Aug 7, 2015 · 10 comments
Open

Matchers do not warn on improper use #160

adamkaplan opened this issue Aug 7, 2015 · 10 comments

Comments

@adamkaplan
Copy link

Clang seems quite confused by the chained properties DSL. Today I found that some of my matchers were missing trailing parens, effectively returning but not executing the matcher. The specific case was .beNil vs .beNil(). The former compiles without any warning and appears to "pass", while in fact doing nothing at all.

This shouldn't happen. It might be a Clang bug since Clang changes it's mind based on the precedence.
screenshot 2015-08-07 17 36 48

I tried enabling some additional Clang flags to no avail. Ideally Expecta shouldn't go quietly into the night with all of your tests running as no-ops...

@adamkaplan
Copy link
Author

I did search first, but then... #149

@orta
Copy link
Member

orta commented Aug 7, 2015

Yeah, so far we've not been able to figure out a way to enforce the () either.

@adamkaplan
Copy link
Author

Worth filing something against Clang? It might be an edge case in the AST.

@orta
Copy link
Member

orta commented Aug 8, 2015

Probably, it's basically accessing the block but doing nothing - something that should raise a warning if it's not inside an if statement.

@adamkaplan
Copy link
Author

This is a real head scratcher! I distilled the problem down to a very simple example for the future humans who will know how to fix this:

https://gist.github.com/adamkaplan/3c69fda47bf02790a860#file-expexpectawarningdemo-m

@modocache
Copy link
Member

This is really interesting. I think this does indeed demonstrate a limitation in Clang. I think it's being tripped up by the expect macro. The following, which uses the _EXP_expect() function instead of a macro, correctly emits a warning when compiled with Clang:

// Emits warning: "Property access result unused - getters should not be used for side effects"
_EXP_expect(nil, 0, "", ^id{ return nil; }).to.beNil;

@adamkaplan
Copy link
Author

Yup, definitely related to the macro expansion. I filed a a bug against Clang last week but forgot to link it:

https://llvm.org/bugs/show_bug.cgi?id=24404

@modocache
Copy link
Member

Oh, awesome! Thanks for filing the bug! 👍

@AlexDenisov
Copy link

Hi there, seems it's done intentionally.
See the comment: https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaStmt.cpp#L199

Though it works as expected when the ShouldSuppress omitted.

@adamkaplan
Copy link
Author

@AlexDenisov The intention per the comments is a few degrees off from observed behavior.

The unused result does not originate within macro body expansion, but rather a function called on the result of the expansion. In this case the function -beNil produces the unused result, not the macro. The way this looks, any code that directly operates on a macro loses some useful diagnostics...

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

No branches or pull requests

4 participants