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 infinite recursion for "each _" #244

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

amv146
Copy link
Contributor

@amv146 amv146 commented Oct 29, 2024

Until we come up with a better solution for evaluating the type of "_" in each expressions, we will need to report the type as "Unknown". Right now, there is an infinite recursion when trying to determine its type

@amv146 amv146 requested a review from JordanBoltonMN October 29, 2024 21:37
@JordanBoltonMN
Copy link
Contributor

Can you add one more test for each [foo] (which should be a defined function that returns unknown).

@@ -42,7 +42,7 @@ import { TypeById } from "../../typeCache";
// Drops PQP.LexSettings and PQP.ParseSettings as they're not needed.
export interface InspectTypeState
extends PQP.CommonSettings,
Omit<InspectionSettings, keyof PQP.Lexer.LexSettings | keyof PQP.Parser.ParseSettings> {
Omit<InspectionSettings, keyof PQP.Lexer.LexSettings | keyof PQP.Parser.ParseSettings> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Odd, my prettier keeps this indented. I don't recall changing it any time recently, but did you run npm install to ensure you have the pinned version?

@@ -515,6 +515,10 @@ export async function dereferencedIdentifierType(
return Type.AnyInstance;
}

if (deferencedLiteral === "_") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid this will break let _ = 1 in 1, which is valid Power Query, and something we can properly type.

@@ -111,6 +111,23 @@ describe(`Inspection - Type`, () => {
TypeUtils.numberLiteral(false, `1`),
),
));

it(`each _`, async () =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind adding a test for each [foo] and each _[foo] to cover field access as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I'm not going to block this PR on that. I'll do a follow-up change.

@amv146
Copy link
Contributor Author

amv146 commented Nov 22, 2024

@microsoft-github-policy-service agree company="Microsoft"

@amv146 amv146 merged commit 1747beb into master Nov 22, 2024
4 checks passed
@amv146 amv146 deleted the dev/alexvallone/fix-infinite-each-recursion branch November 22, 2024 19:14
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