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

Major change making dsymbol deduce ufcs #724

Merged
merged 9 commits into from
Mar 16, 2023
Merged

Major change making dsymbol deduce ufcs #724

merged 9 commits into from
Mar 16, 2023

Conversation

vushu
Copy link
Contributor

@vushu vushu commented Mar 13, 2023

I have moved some utils to dsymbol.utils since it is used for ufcs deduction, and made dsymbol handle it all.
Doing this will separate the responsibility of UFCS completion.

Copy link
Member

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

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

is it possible we split this into multiple smaller PRs? This is pretty hard to review this way otherwise

@vushu
Copy link
Contributor Author

vushu commented Mar 13, 2023

@WebFreak001 sorry about the big PR I will see what I can do

@vushu
Copy link
Contributor Author

vushu commented Mar 13, 2023

Seem like it's a bit hard to split up since it include some refactoring and some new test additions, I have tried to split it up in some commits that should explain the change.

the biggest commit is c4b8a74

but the others aren't so big.
I will try to keep the PR's smaller in the future.

@WebFreak001
Copy link
Member

what did the most recent commit 6c66afd fix? Should probably get a unittest

@vushu
Copy link
Contributor Author

vushu commented Mar 14, 2023

Yeah think I will investigate it.

@vushu
Copy link
Contributor Author

vushu commented Mar 14, 2023

Was a false alarm, I just now mark the symbols as CompletionKind.ufcsName.

Copy link
Member

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

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

review round 1

tests/tc_erroneous_body_content/expected1.txt Outdated Show resolved Hide resolved
tests/tc_incomplete_switch/expected1.txt Outdated Show resolved Hide resolved
dsymbol/src/dsymbol/conversion/package.d Show resolved Hide resolved
src/dcd/server/autocomplete/complete.d Outdated Show resolved Hide resolved
@@ -306,6 +308,8 @@ AutocompleteResponse parenCompletion(T)(T beforeTokens,
auto expression = getExpression(beforeTokens[0 .. $ - 1]);
response.setCompletions(pair.scope_, expression,
cursorPosition, CompletionType.calltips, beforeTokens[$ - 1] == tok!"[");
response.completions ~= pair.ufcsSymbols.map!(s => makeSymbolCompletionInfo(s, s.kind)).array;
response.completionType = CompletionType.calltips;
Copy link
Member

Choose a reason for hiding this comment

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

why this change? calltips doesn't seem right here and change is unrelated to the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will clarify this with a comment.

dsymbol/src/dsymbol/ufcs.d Outdated Show resolved Hide resolved

if (sortedBeforeTokens.length >= 2
&& sortedBeforeTokens[$ - 1].type is tok!"."
&& sortedBeforeTokens[$ - 2].type is tok!"identifier")
Copy link
Member

Choose a reason for hiding this comment

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

what about a prefix like foo().? This currently only matches identifier., but there are a bunch more ways how dot completion could be called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup not yet implemented, which is what is needed for ufcs function chaining.

auto slicedAtParen = sortedBeforeTokens[0 .. index];
if (slicedAtParen.length >= 4
&& slicedAtParen[$ - 4].type is tok!"identifier"
&& slicedAtParen[$ - 3].type is tok!"."
Copy link
Member

Choose a reason for hiding this comment

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

ditto for this may be foo(x).ufcsBar(y)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

function chaining isn't yet supported, which is why I needed the return type implementation earlier

dsymbol/src/dsymbol/ufcs.d Outdated Show resolved Hide resolved
dsymbol/src/dsymbol/ufcs.d Outdated Show resolved Hide resolved
@vushu
Copy link
Contributor Author

vushu commented Mar 16, 2023

I have done the first round of request changes, and added some null checks for when checking willImplicitBeUpcasted and non constrained templates.

@WebFreak001
Copy link
Member

CI failing

@vushu
Copy link
Contributor Author

vushu commented Mar 16, 2023

Forgot to include some updated tests.

Copy link
Member

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

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

sorry, didn't realize the destroy would introduce memory corruption before, can you just fix these remaining things?

dsymbol/src/dsymbol/ufcs.d Outdated Show resolved Hide resolved
dsymbol/src/dsymbol/conversion/package.d Outdated Show resolved Hide resolved
@WebFreak001 WebFreak001 merged commit 24f67f3 into dlang-community:master Mar 16, 2023
@vushu
Copy link
Contributor Author

vushu commented Mar 16, 2023

I make another PR for the change. Ah just saw you already fixed it thanks.

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

Successfully merging this pull request may close these issues.

2 participants