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

fix: Refactor the way of collecting LSP information #110

Merged
merged 316 commits into from
Mar 29, 2021

Conversation

michalbali256
Copy link
Contributor

Redesign the way information needed for go to definition, completion, etc are collected.
Whole statements are analyzed once they were parsed, instead of collecting the information in various places during parsing. Using visitor pattern to keep the analysis of operands in one place.
LSP context keeps all the occurences and definitions structured according to file and macro scope.

…factor

# Conflicts:
#	parser_library/src/analyzer.cpp
#	parser_library/src/context/hlasm_context.cpp
#	parser_library/src/workspaces/workspace.h
#	parser_library/test/context/data_attribute_test.cpp
#	parser_library/test/context/macro_test.cpp
#	parser_library/test/context/ord_sym_test.cpp
#	parser_library/test/copy_mock.h
#	parser_library/test/expressions/expr_mocks.h
#	parser_library/test/processing/lookahead_test.cpp
#	parser_library/test/processing/opsyn_test.cpp
Comment on lines 160 to 169
std::unordered_map<parser_library::completion_item_kind, lsp_completion_item_kind> completion_item_kind_mapping()
{
using namespace parser_library;
return { { completion_item_kind::mach_instr, lsp_completion_item_kind::function },
{ completion_item_kind::asm_instr, lsp_completion_item_kind::function },
{ completion_item_kind::ca_instr, lsp_completion_item_kind::function },
{ completion_item_kind::macro, lsp_completion_item_kind::file },
{ completion_item_kind::var_sym, lsp_completion_item_kind::variable },
{ completion_item_kind::seq_sym, lsp_completion_item_kind::reference } };
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a single static const map.

{ "insertText", item.insert_text() } });
}
to_ret = json { { "isIncomplete", completion_list.is_incomplete() }, { "items", completion_item_array } };
to_ret = json { { "isIncomplete", false }, { "items", completion_item_array } };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we perhaps impose some limits here and return isIncomplete: true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you noticed any performance issues or completion latency? Either way, I would add it to a Todo list for the future

parser_library/src/context/hlasm_context.cpp Outdated Show resolved Hide resolved
parser_library/src/expressions/mach_expr_visitor.h Outdated Show resolved Hide resolved
parser_library/src/workspace_manager.cpp Outdated Show resolved Hide resolved
@@ -15,6 +15,7 @@
#ifndef HLASMPLUGIN_PARSERLIBRARY_LIBRARY_H
#define HLASMPLUGIN_PARSERLIBRARY_LIBRARY_H

#include <regex>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps split off library_local

parser_library/src/workspaces/workspace.cpp Outdated Show resolved Hide resolved
Comment on lines +267 to +271
auto opencodes = find_related_opencodes(document_uri);
if (opencodes.empty())
return { pos, document_uri };
// for now take last opencode
return opencodes.back()->get_lsp_feature_provider().definition(document_uri, pos);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we only need a function that returns that last element?

Comment on lines 19 to 23
for (const auto& i : actual) \
{ \
auto it = std::find(expected.begin(), expected.end(), i); \
EXPECT_NE(it, expected.end()) << "Test returned unexpected value:" << i; \
} \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not work for {1,1} and {1,2}

I suggest sorting if possible and then compare in order.

Also expected cannot be an expression, because it would be evaluated multiple times, so I suggest a helper functions.

@michalbali256 michalbali256 marked this pull request as ready for review March 23, 2021 19:18
michalbali256 and others added 9 commits March 25, 2021 17:59
Co-authored-by: slavek-kucera <53339291+slavek-kucera@users.noreply.github.com>
…factor

# Conflicts:
#	parser_library/include/c_view_array.h
#	parser_library/include/lib_config.h
#	parser_library/include/protocol.h
#	parser_library/src/analyzing_context.h
#	parser_library/src/debugging/debugger.cpp
#	parser_library/src/debugging/debugger.h
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 57 Code Smells

94.4% 94.4% Coverage
0.2% 0.2% Duplication

@michalbali256 michalbali256 merged commit d767b6d into development Mar 29, 2021
@michalbali256 michalbali256 deleted the lsp-context-refactor branch April 14, 2021 09:43
@github-actions
Copy link

🎉 This PR is included in version 0.13.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Jun 1, 2021

🎉 This PR is included in version 0.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants