I found this by visual inspection while trying to get up-to-speed on modules, so I'm still uncertain how to test it. If the current one is really the actual behaviour, then parseOptionalAttribute() can return void.
makes sense. can you include a testcase? I assume we are forgetting some sort of diagnostic without this patch.
Also, if learning about Clang's modules, I would start with addHeaderInclude in lib/Frontend/FrontendActions.cpp. The module map stuff ultimately boils down to that (essentially, it textually forms a header which it then more-or-less compiles as a PCH; the submodule stuff is only relevant for setting Decl::Hidden and some extra checking when importing).
Added a test. Yes, with the patch we stop immediately if we fail to parse attributes, while the old code kept emitting diagnostics. I like the new behaviour better, but I'll defer the decision to you.