[PATCH] preview patch for fixit for finding modules needing import/inclusion

[Richard Smith]

On Wed, Mar 26, 2014 at 4:19 PM, Thompson, John <
John_Thompson at playstation.sony.com> wrote:

> Richard,
>
> Thanks for looking at this.  I'll rework it.
>
> > We should probably only do this if we got to the end of typo-correction
> and found no corrections.
>
> I'm not so sure about this.  For example, in my test case, it would
> suggest "use_this2" as a typo fixit for "user_this1", instead of finding
> the symbol in the missing module import.  I guess it's a question of
> whether an exact match should take precedent over a partial match.  What do
> you think?  I'll follow your lead.


I think a 'exact match, but missing import' correction is vastly more
likely to be the one the user meant than a 'near match' correction, but I'm
worried about the cost of doing this check. I suppose once we've committed
to taking the performance cost, it's better to do this lookup earlier.

Incidentally, I think you don't actually need to perform a lookup in your
global module index code path -- just loading all the modules should be
enough. Then the normal logic for finding hidden names from imported
modules should find the name.

> It looks like this will build every module described in any module map
> we've loaded. That could be extremely time-consuming, and could produce
> errors. Is that what you want here?
>
> Yeah, that's a key question.  The problem is that the global module index
> only has symbols for modules that clang with the current argument hash has
> built modules before, meaning that if someone forgets an import, they won't
> see this fixit unless they've imported the module in some other compile
> with the same hash.  So the key options are:
>
> 1. Don't build a full global module index.  User only sees the fixit for
> modules already in the global module index.
> 2. When a symbol can't be resolved, create the full global module index on
> demand.  (My current implementation.)
> 3. Build a full global module index the first time or whenever the
> module.map file or headers change.  Might be a slow build when this
> triggers.
>
> What do you think?
>

One use case we want to support has (almost) all header files in modules,
including user headers that change frequently, so (3) is likely to be
expensive for that use case, if I understand what you're proposing.

I'm torn between options 1 and 2. Mostly, I'm worried about the
unpredictability of option 1.


One other issue I just thought of: loading modules affects the semantics of
the translation unit, even if those modules aren't made visible. Because a
module is considered to be part of the program if it's loaded into a
translation unit, we'll diagnose mismatched redeclarations. For instance,
if this is in a module that's in one of the module maps on your include
path:

  int foo;

... and you hit a typo correction and then see this:

  void foo();

... you're likely to get a 'redefinition as a different kind of entity'
diagnostic.

This is not ideal. The big problem is that we sometimes call CorrectTypo
when we're not sure that an error has occurred! You can find these cases
easily enough: look for cases where ErrorRecovery == false in the call to
diagnoseTypo. I suppose you could add an ErrorRecovery flag to CorrectTypo
too, and only do your global module index import in the (common) error
recovery case.

-----Original Message-----
> From: cfe-commits-bounces at cs.uiuc.edu [mailto:
> cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Richard Smith
> Sent: Wednesday, March 26, 2014 3:25 PM
> To: dgregor at apple.com; richard at metafoo.co.uk;
> john.thompson.jtsoftware at gmail.com
> Cc: cfe-commits at cs.uiuc.edu
> Subject: Re: [PATCH] preview patch for fixit for finding modules needing
> import/inclusion
>
>
>   This looks like basically the right approach to me.
>
>
> ================
> Comment at: lib/Sema/SemaLookup.cpp:4060-4061 @@ -4056,1 +4059,4 @@
>
> +  // Look for the symbol in non-imported modules.
> +  if (getLangOpts().Modules && getLangOpts().ModulesSearchAll) {
> +    // Try to find the type in modules via the global module index.
> ----------------
> We should probably only do this if we got to the end of typo-correction
> and found no corrections.
>
> ================
> Comment at: lib/Sema/SemaLookup.cpp:4096-4098 @@ +4095,5 @@
> +            TC.setCorrectionDecl(ModRes.getAsSingle<NamedDecl>());
> +          // Hide the module again. diagnoseTypo will unhide the decl
> module.
> +          Loader.makeModuleVisible(TheModule, Module::Hidden,
> +            TypoName.getLoc(), false);
> +        }
> ----------------
> This won't work. We don't support visibility decreasing. Instead, you
> should be able to just load the module rather than making it visible, and
> tell your `LookupResult` to find hidden results during the lookup.
>
> ================
> Comment at: lib/Frontend/CompilerInstance.cpp:1440
> @@ +1439,3 @@
> +                   // Load a module as hidden.  This also adds it to the
> global index.
> +        ModuleLoadResult Result = loadModule(TheModule->DefinitionLoc,
> Path,
> +          Module::Hidden, false);
> ----------------
> It looks like this will build every module described in any module map
> we've loaded. That could be extremely time-consuming, and could produce
> errors. Is that what you want here?
>
> ================
> Comment at: lib/Serialization/GlobalModuleIndex.cpp:345-358
> @@ -344,1 +344,16 @@
>
> +void GlobalModuleIndex::dump() {
> +  std::fprintf(stderr, "*** Global Module Index Dump:\n");
> +  std::fprintf(stderr, "Module files:\n");
> +  for (llvm::SmallVector<ModuleInfo, 16>::iterator I = Modules.begin(),
> +      E = Modules.end(); I != E; ++I) {
> +    ModuleInfo *MI = (ModuleInfo*)I;
> +    std::fprintf(stderr, "** %s\n", MI->FileName.c_str());
> +    if (MI->File)
> +      MI->File->dump();
> +    else
> +      std::fprintf(stderr, "\n");
> +  }
> +  std::fprintf(stderr, "\n");
> +}
> +
> ----------------
> Feel free to just check this part in.
>
>
> http://llvm-reviews.chandlerc.com/D2671
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140326/068354ce/attachment.html>