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 CLI analysis reporting wrong file names #146

Merged
merged 4 commits into from
Nov 6, 2021
Merged

Conversation

Rerumu
Copy link
Contributor

@Rerumu Rerumu commented Nov 6, 2021

Fix #144

Regarding (#144 (comment)):

We should check frontend.isDirty(name) in analyzeFile before running type checking. This will automatically skip redundant checks for dependent modules when we encounter them again in recursive traversal

At about which point should this check happen? From my understanding frontend.check(name) does type checking, so should the function just exit early reporting no errors?

Additionally, reportWarning at a glance has the same undesirable behavior that reportError did, but I opted to wait and ask if this is intentional before changing it also.

@zeux
Copy link
Collaborator

zeux commented Nov 6, 2021

At about which point should this check happen? From my understanding frontend.check(name) does type checking, so should the function just exit early reporting no errors?

I believe Frontend propagates errors for dependent modules for type checking, so .check will return dependent module info, but lint doesn't - so if the module is not dirty, we should skip .check but still run .lint.

@Rerumu
Copy link
Contributor Author

Rerumu commented Nov 6, 2021

If that's correct then that just leaves whether reportWarning should be changed to respect module names too.

@zeux
Copy link
Collaborator

zeux commented Nov 6, 2021

Not sure what you mean - LintWarning doesn't carry a module name?

In general this silent error propagation business is pretty weird and it would probably be better to refactor the Frontend to not do that, and instead to expose dependent modules to the caller so that the caller can aggregate errors if need be. However, this would change the Frontend interface and we have internal tools that rely on the specific behavior, so we'd need to do that internally separately.

@Rerumu
Copy link
Contributor Author

Rerumu commented Nov 6, 2021

My bad on that, I was looking at TypeError not LintWarning. I think the issue is solved then.

CLI/Analyze.cpp Outdated Show resolved Hide resolved
CLI/Analyze.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@zeux zeux left a comment

Choose a reason for hiding this comment

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

Thanks!

@zeux zeux merged commit 96b1707 into luau-lang:master Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

luau-analyze reports errors from other files.
2 participants