Page MenuHomePhabricator

preview patch for fixit for finding modules needing import/inclusion

Authored by jtsoftware on Jan 31 2014, 5:19 PM.



This is the beginnings of a patch to add a fixit for automatically loading a module to resolve a type or symbol reference and outputting a fixit message.

I wanted to get some early feedback to make sure I'm on the right path, and also some answers for questions about a couple of difficult areas.

A problematic part is that the global module index seems to only include modules that have been imported before, and has not necessarily already been created or loaded yet. To handle this, I added a loadGlobalModuleIndex to ModuleLoader that will create the module index if it doesn't yet exist, and will walk the top level modules in the module map and load any modules not already loaded (it will load them as hidden), for the side effect of loading the global module index with identifiers from all the modules. This might involves two calls to create the index, as I can't compare it against the module map unless I have an index already. This is one area I need feedback from someone who knows the module system, as there might be a better way. I suppose we don't create a full index right off, because of the performance cost.

Then, in Sema I added a getTypeNameWithModuleCheck function to be used instead of getTypeName, which, if the type is not found, will do the load global index check, look up in the index any modules with the type identifier, and if found, make the module visible, redo the type search, and if found, display the fixit and continue. If the type is not found, the module is made hidden again.

Another probable problem with this scheme is that apparently only top-level modules correspond to a module AST file, and the global module index only lists those top-level modules. Is there code somewhere to find the module a symbol belongs to such that I can just make that module visible?

There's some other FIXME's in the code, such as one for figuring out the SourceLocation for where to put the fixit. Any suggestions?

Perhaps the fixit message itself needs some revision, I just took a stab at it, as I wanted to get something out for feedback.

I added -fmodules-search-all and -fno-modules-search-all to enable or disable this (enabed by default). I also added a need-import warning group.

This affected a few of the Modules tests. I punted on it by just added a -fno-modules-search-all option to their command lines.

I added one new bare-bones test for this fixit, in test/Modules/undefined-type-fixit1.cpp. More needs to be done to it to check the various scenarios of the global module index existing.

Because of the addition of an abstract function to ModuleLoader, I needed to put a stub for it in some unit tests that derive from it.

The formatting might be a bit spotty, as the focus here is to get some early feedback.

When and if this fixit and the infrastructure is good enough, then I can find other places to do similar fixit's for resolving identifiers.



Diff Detail

Event Timeline

rsmith added inline comments.Mar 4 2014, 6:15 PM

I'm not fond of this approach -- we'll end up with these WithModuleCheck functions scattered throughout various places that perform name lookup, with lots of duplication.

The existing recovery for missing imports is performed in Sema::CorrectTypo; I would suggest putting the new functionality there too. Perhaps if typo correction fails to find a result using its normal resolution mechanism, you could pull in all the modules that the global index is aware of, and try again?

697–706 ↗(On Diff #6814)

I'd like for this to be split out into a separate change. I also find this a little surprising, so I'd like an explanation of why this is the appropriate thing to do. If we're using -I-, we should probably still look for the contents of a module relative to the module. If this is fixing the bug I think it's fixing, I think the problem is that we put the wrong paths into the synthetic file containing #includes, and fixing it there would be better.

jtsoftware updated this revision to Unknown Object (????).Mar 26 2014, 3:06 PM

I've reworked this to do the check in CorrectTypo as suggested. This is just a partial post, as I've omitted needed changes to other tests so we can focus on the key parts. I basically would like to know if this is the right approach, and what needs to be done to make it right/better.

Question: I saw that there was some code for handling missing imports, but I couldn't see where it searched the global module index, and my new test case (with the new code disabled) didn't trigger it. Was it just for some special cases?



This looks like basically the right approach to me.


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?


We should probably only do this if we got to the end of typo-correction and found no corrections.


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.

345–358 ↗(On Diff #8146)

Feel free to just check this part in.

jtsoftware updated this revision to Unknown Object (????).Apr 1 2014, 11:11 AM

I added a BuildingModule flag to ModuleLoader so I could avoid doing the correctTypo modules import fixit or global module index rebuild when a module is being built.

I merged the prior need import error message into a single warning.

I hack the test/Modules tests to accomodate the new error, and to disable the modules import fixit for tests that trigger an index rebuild involving the test/Modules/ file, as some modules have build errors that get triggered.

I didn't do anything with respect to the issue of not displaying error messages during a module build, leaving it for a separate fix.

Is this ready to check in?



rsmith added inline comments.Apr 11 2014, 3:52 PM
6961–6963 ↗(On Diff #8275)

This shouldn't be a Warning. If we really want to allow this as an extension (and I don't see why we should), it should be an ExtWarn, and should be DefaultError.


Only the non-default value of the flag should have HelpText.


This constructor should be explicit.

Also, just BuildingModule, not BuildingModuleFlag.


Instance(/*BuildingModule=*/true); would be more obvious.


Do we need to take any other steps to ensure we've actually loaded all the modulemap files that are within our include paths?


I think this should default to off. It still makes diagnostics non-reproducible (because it depends on what other modules happen to be in the global index at the point when we perform typo correction), and may have a *very* significant runtime penalty if we hit a diagnostic.


I really don't like this collection of unexplained mystery arguments (repeating the default arguments for all but the last).

Maybe move the error recovery flag earlier, and change it to an enum class so its values can have more obvious names (and won't accidentally convert to bool)?


&& should go on the previous line.


I think this is all unnecessary; just make sure we've loaded all the modules that contain the identifier, and let the normal recovery for an invisible name do the rest.


You can't put the genie back in the bottle: once a module is unhidden, hiding it again doesn't work.


This is not a correct fixit.


This is also not OK. Please take out this fixit code for now; that's essentially orthogonal and we can deal with it as a separate change.

345–358 ↗(On Diff #8275)

Please commit this as a separate change.


If we really want to allow this as an extension (and I don't see why we should), it should be an ExtWarn, and should be DefaultError.

Do mean you're pretty much against this fixit, I suppose because of the performance impact when it kicks in?


jtsoftware updated this revision to Unknown Object (????).Apr 18 2014, 5:15 PM

Here's another crack at it, per the review changes, except for checking to see if all module maps in the search paths are loaded, which I can address later. Basically, I reverted back to the original error messages, reduced the symbol lookup to let the latter code handle it. I left in the global module index search so that those symbols found would get precedence over the normal typo checks. This option needs to be enabled via the -fmodules-search-all option. Is this the correct classification of the message, as errors?

I have quite a few comments, but they're just typographical issues and things that can be deferred to later patches; this looks more-or-less ready to go.

Please also add a paragraph or so to docs/Modules.rst explaining this flag and what it does.


No need for abbreviation here; CorrectTypoKind would be preferred.


The usual convention is to form an initialism from the name of the enum, then add underscore then enumerator names. If this enum is named CorrectTypoKind, these'd be CTK_NonError and CTK_ErrorRecovery.


Please use (/*BuildingModule=*/true) here.


I assume this means that -fmodules-search-all won't search all modules if we see an error while building a module? (That's fine, but it's probably worth putting this into the documentation, at least.)


This will only cover module.modulemap files that have already been found and loaded. That seems fine (we don't really want to go trawling the file system looking for them), but again, it's something to be aware of.


Name this RecreateIndex, per the coding standard.


This is unused; please remove.


The ModuleMap iterators will only give you top-level modules, so you don't need this loop.


The SourceLocation you're using here doesn't make much sense to me. Maybe you could pass a location into this function (the location at which we're performing the typo correction)?


This line is too indented; it looks like it was intended to be part of the for loop, even though it wasn't.


It'd be useful to have some way to tell loadModule to suppress diagnostics from the submodule build (and to not leave a file behind if building it failed, so that later imports will try to build it again and get the diagnostics).

That can wait for a later patch, though.


Please indent this to line up with the character after the (.


We generally try to keep Sema from using Lex headers. More on how you might approach this below.


I feel that this could be layered better (it seems like Sema should have a callback that CompilerInvocation fills in with this code), but again, I'm OK with that being deferred to a later patch.


We deliberately don't do this for the current code to search unimported submodules -- we assume that it's not very likely that a name is both mistyped and from an unimported module. See TypoCorrectionConsumer::FoundDecl.

For the unimported top-level module case, this would also defeat the desire to use the global index to minimize the number of modules we search, so I think we have convincing reasons to not attempt this.

jtsoftware accepted this revision.May 5 2014, 6:14 PM
jtsoftware added a reviewer: jtsoftware.

Committed in r206977 (plus fixes r206981 r207011).

6961–6963 ↗(On Diff #8275)

Richard, does this mean you're pretty much against this, probably because of the performance hit if it kicks in?


I don't know. I'll have to dig into it, as I just assumed they were all loaded. Can I address this later?

This revision is now accepted and ready to land.May 5 2014, 6:14 PM
jtsoftware closed this revision.May 5 2014, 6:14 PM

Committed in r206977 (plus fixes r206981 r207011).

Sorry, I was too aggressive in cleaning out my Phabricator list, and closed this one by mistake. Though I did an initial check-in of it, I'm still working on a couple of the review issues Richard said could be postponed to a later check-in.