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

Excluded files shouldn't be included in the symbol resolution. #4095

Closed
smwikipedia opened this issue Aug 16, 2019 · 15 comments
Closed

Excluded files shouldn't be included in the symbol resolution. #4095

smwikipedia opened this issue Aug 16, 2019 · 15 comments
Labels
Feature: Go to Definition An issue related to Go to Definition/Declaration. fixed Check the Milestone for the release in which the fix is or will be available. Language Service quick fix verified Bug has been reproduced
Milestone

Comments

@smwikipedia
Copy link

smwikipedia commented Aug 16, 2019

Type: LanguageService

Describe the bug
I have a folder containing below files. The func1.c and func2.c contains 2 functions definitions. They are for the same function name but with different implementation.

Below are the folder/file details.

image

(func.h)

void func();

(func1.c)

void func()
{
    int a;
    a++;
}

(func2.c)

void func()   // Same function name but with different implementation
{
    int b;
    b++;
}

(main.c)

#include "func.h"
int main()
{
    func();
}

I use files.exclude to exclude one of the c files, func2.c from workspace. And the func2.c disappeared from the workspace explorer indeed.

(settings.json)

{
    "files.exclude": {
        "func2.c": true
    }
}

I expect the file parsing / symbol resolution should do the same exclusion when looking for definition for func().

But actually, after I reset the intellisense database, definitions from both func1.c and func2.c are still listed.

image

  • OS and Version:

  • VS Code Version:
    Version: 1.37.0 (user setup)
    Commit: 036a6b1d3ac84e5ca96a17a44e63a87971f8fcc8
    Date: 2019-08-08T02:33:50.993Z
    Electron: 4.2.7
    Chrome: 69.0.3497.128
    Node.js: 10.11.0
    V8: 6.9.427.31-electron.0
    OS: Windows_NT x64 10.0.16299

  • C/C++ Extension Version:
    0.25.0-insiders2

  • Other extensions you installed (and if the issue persists after disabling them):

  • A clear and concise description of what the bug is.

To Reproduce

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Expected behavior

Screenshots

Additional context

@sean-mcmanus
Copy link
Collaborator

Can you try running the Rescan Workspace command? We might have a bug with updating the symbols right away after the files.exclude changes.

@sean-mcmanus
Copy link
Collaborator

Oh, actually, we disable file-based excludes by default. You can re-enable that via changing the C_Cpp.exclusionPolicy setting.

smwikipedia added a commit to smwikipedia/zephyr2vsc that referenced this issue Aug 16, 2019
@smwikipedia
Copy link
Author

smwikipedia commented Aug 16, 2019

@sean-mcmanus (Thanks for your quick response!)

Adding "C_Cpp.exclusionPolicy": "checkFilesAndFolders", can solve the problem in my previous scenario.

But for below scenario, the issue remains.

What I change:

  • The .c and .h file content are the same.
  • I only change the folder structure and the settings.json.
  • And add a c_cpp_properties.json.

Folder/file structure:

image

(settings.json)

{
    "files.exclude": {
        "folder1/func2.c" : true
    },
    "C_Cpp.exclusionPolicy": "checkFilesAndFolders",
    "C_Cpp.intelliSenseEngine": "Default",
    "cmake.configureOnOpen": false
}

Then we can see the folder1/func2.c disappeared from the Workspace Explorer indeed:

image

(c_cpp_properties.json)
Add folder1 to the browse.path.

{
    "configurations": [
        {
            "name": "Win32",
            "includePath": [
                "${workspaceFolder}/**"
            ],
            "defines": [
                "_DEBUG",
                "UNICODE",
                "_UNICODE"
            ],
            "windowsSdkVersion": "8.1",
            "compilerPath": "C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/cl.exe",
            "cStandard": "c11",
            "cppStandard": "c++17",
            "intelliSenseMode": "msvc-x64",
            "browse": {
                "limitSymbolsToIncludedHeaders": true,
                "databaseFilename": "${workspaceFolder}/.vscode/browse.zephyr.db",
                "path": ["folder1"]
            }
        }        
    ],
    "version": 4
}


Then reset the intellisense DB and rescan the workspace.

The Goto definition for func() will still list 2 candidates.

image

It seems the browse.path doesn't take into consideration the files.exclude.
According to below description. I think it should.
image

And btw, I am not using tag parser. I am using the default intellisense engine.

image

@sean-mcmanus
Copy link
Collaborator

Hmm...I'm not reproing the bug. The files.exclude is correctly ignoring the browse.path.

Are you able to view the debug logs (https://code.visualstudio.com/docs/cpp/enable-logging-cpp) to see

@Colengms @michelleangela Can any of you repro this?

@sean-mcmanus sean-mcmanus added Language Service more info needed The issue report is not actionable in its current state not reproing We're not able to reproduce the issue (it's unlikely to get fixed until we find one). labels Aug 17, 2019
@smwikipedia
Copy link
Author

smwikipedia commented Aug 17, 2019

As far as the workspace explorer is concerned, the files.exclude successfully ignore the browse.path. But as to the symbol resolving, the result is incorrect.

The folder1/func2.c is excluded by “files.exclude”. But browser.path bring back for symbol resolving.

@smwikipedia
Copy link
Author

smwikipedia commented Aug 18, 2019

(@sean-mcmanus , @Colengms , @michelleangela )

I create a repo to reproduce this issue: https://github.com/smwikipedia/fileExclude
There are 2 unexpected results.

To ease the mind, I draw an illustration of the general folder structure. There are 2 alternative calls for the same function named func() from main.c to the func1.c or func2.c. This is quite often to see where different implementation is chosen depending on the build configuration.

image

Steps:

  1. Clone the repo.
    With the initial clone, the browse.path entry in c_cpp_properties.json file is []. Shown as below:
    image

    And the folder1/func2.c is not shown in Workspace Explorer because it is explicitly excluded with below entry in settings.json.
    image

  2. Open the repo folder in VS Code.

  3. Double click the folder2/main.c in Workspace Explorer to open it.

  4. Move the cursor to the func() invocation in folder2/main.c at line 4.

  5. Press F12 or right click the Go to definition.

  6. It will only jump to the func() declaration in folder2/func.h . (this is the first unexpected result. I expected it should jump to the folder2/func1.c since this file is already in the workspace and VS Code should well know about it.)

  7. Double click the folder1/func1.c in Workspace Explorer to open it. (It seems this step reminds the VS Code about this file.)

  8. Repeat step 4~5, this time it will jump correctly to the func() definition in folder1/func1.c.

  9. Modify the c_cpp_properties.json file line 21 to below:
    image

  10. Then repeat step 4~5, this time, both folder1/func1.c and folder1/func2.c will be listed for the definition of func(). Shown as below. This is the second unexpected result. I expected only folder1/func1.c should be jumped to since folder1/func2.c has been explicitly excluded with files.exclude entry in settings.json. But it seems step 9 brought it back for symbol resolving.

image

  1. Undo step 9 by removing the folder1 in browse.path entry. Shown as below:
    image

  2. Repeat step 4 and 5 ( you may need to repeat step 7 as well). The func() definition in folder1/func1.c can be correctly jumped to.

@sean-mcmanus sean-mcmanus self-assigned this Aug 19, 2019
@sean-mcmanus
Copy link
Collaborator

sean-mcmanus commented Aug 19, 2019

@smwikipedia Thanks for the detailed repro steps.

The issue in No. 6 is caused by your browse.path being empty ([]) -- that causes us to not parse any files unless they're opened, which is why doing step 7 fixes it for you. Changing that to be "path": [ "${workspaceFolder}" ] fixes it.

Okay, I repro the bug in No. 10 now -- investigating (something is causing the files.exclude check to be skipped) -- it's a case sensitivity bug on Windows.

@sean-mcmanus sean-mcmanus added Feature: Go to Definition An issue related to Go to Definition/Declaration. quick fix verified Bug has been reproduced and removed more info needed The issue report is not actionable in its current state not reproing We're not able to reproduce the issue (it's unlikely to get fixed until we find one). labels Aug 19, 2019
@sean-mcmanus sean-mcmanus added this to the 0.25.0 milestone Aug 19, 2019
@sean-mcmanus sean-mcmanus added the fixed Check the Milestone for the release in which the fix is or will be available. label Aug 19, 2019
@sean-mcmanus sean-mcmanus removed their assignment Aug 19, 2019
@smwikipedia
Copy link
Author

smwikipedia commented Aug 20, 2019

@sean-mcmanus Many thanks for the response.

The reason I didn't put "${workspaceFolder}" in browse.path is that I want to reduce the scope that VS Code needs to scan for symbols. I want VS Code to only scan the folders I put it in the browse.path. I guess this is a natural thing to do.

As I tried, putting the ${workspaceFolder} in browse.path greatly increased the number of files to parse when creating the symbol DB. In my scenario, the parsing time increase from less than 1 min to more than 5 mins. And the symbol DB size increase from 25M to 226M. I can live up with the big DB size. But it is really frustrating to wait for more than 5 mins before the symbols can be resolved.

And even more, putting the ${workspaceFolder} in browse.path will cause more explicitly excluded files being brought back for symbol resolution, causing much ambiguity. As shown below:

image

Could you tell me more about the Windows sensitivity bug so I may work around it for now? Or should I wait for the new release?

@sean-mcmanus
Copy link
Collaborator

The bug we fixed for our pending release (either Tuesday or Wednesday) is the bug with files.exclude failing to work.

I don't understand what other change is desired. You can use "${workspaceFolder}/*" if you want the browse.path to be non-recursive. Then you can add whatever subfolders you want to the browse.path to add more paths for the symbols you care about finding.

The database should normally be saved after a reload and not require reparsing.

smwikipedia added a commit to smwikipedia/zephyr2vsc that referenced this issue Aug 20, 2019
…ms I cannot leave the browse.path to []. This issue can only be perfectly fixed on VS Code side. I have to wait.
@smwikipedia
Copy link
Author

smwikipedia commented Aug 20, 2019

@sean-mcmanus

In my scenario, I didn't put either ${workspaceFolder} or ${workspaceFolder}/* in the browse.path because I want to be explicit. I don't want to put any unnecessary folders there. (Do I have to?)

I only put the folders I care about in it. These folders are those containing files that indeed participated in a build.

If the fix can make browse.path take into consideration of the files.exclude when parsing symbols. I think it's OK.

@sean-mcmanus
Copy link
Collaborator

You don't need ${workspaceFolder} in the path, but it probably should not be blank, i.e. you could use "folder2" or "${workspaceFolder}/folder2".

@smwikipedia
Copy link
Author

I guess you mean ${workspaceFolder}/folder1 because that's the folder containing func() definition file.

I put the ${workspaceFolder}/folder1 in the browse.path. It cause the same issue as I put just folder1 there.

image

The excluded func2.c is still found.

image

I believe the fix to make browse.path take into consideration of the files.exclude can solve this.

I will wait for the new release.

smwikipedia added a commit to smwikipedia/zephyr2vsc that referenced this issue Aug 20, 2019
@sean-mcmanus
Copy link
Collaborator

Should be fixed with 0.25.0.

@smwikipedia
Copy link
Author

smwikipedia commented Aug 22, 2019

@sean-mcmanus

Unfortunately, the issue is not fully fixed.

If use path separator /, the issue seems fixed.
image

If use path separator \\, the issue remains.

image

image

smwikipedia added a commit to smwikipedia/zephyr2vsc that referenced this issue Aug 22, 2019
… it. We can only use "/" for path separator. "\\" still not working.

microsoft/vscode-cpptools#4095
@sean-mcmanus
Copy link
Collaborator

@smwikipedia Looks like the \\ issue has been a bug for a long time. It should be fixed in our next update.

smwikipedia added a commit to smwikipedia/zephyr2vsc that referenced this issue Aug 30, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Feature: Go to Definition An issue related to Go to Definition/Declaration. fixed Check the Milestone for the release in which the fix is or will be available. Language Service quick fix verified Bug has been reproduced
Projects
None yet
Development

No branches or pull requests

2 participants