specta / expecta

A Matcher Framework for Objective-C/Cocoa
MIT License
1.59k stars 158 forks source link

Matchers do not warn on improper use #160

Open adamkaplan opened 9 years ago

adamkaplan commented 9 years ago

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 commented 9 years ago

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

orta commented 9 years ago

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

adamkaplan commented 9 years ago

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

orta commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

Oh, awesome! Thanks for filing the bug! :+1:

AlexDenisov commented 9 years ago

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 commented 9 years ago

@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...