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

Analyzer: Excluded non-Function methods from BindingAnalyzer #2244

Merged
merged 3 commits into from
Aug 11, 2022

Conversation

amdeel
Copy link
Contributor

@amdeel amdeel commented Aug 10, 2022

This PR updates the BindingAnalyzer to only run analysis when the method being analyzed is located inside a Function.

resolves #2222

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation PR is ready to merge and referenced in pending_docs.md
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)
  • My changes do not require any extra work to be leveraged by OutOfProc SDKs
    • Otherwise: That work is being tracked here: #issue_or_pr_in_each_sdk
  • My changes do not change the version of the WebJobs.Extensions.DurableTask package
    • Otherwise: major or minor version updates are reflected in /src/Worker.Extensions.DurableTask/AssemblyInfo.cs

@amdeel amdeel requested review from bachuv and cgillum August 10, 2022 20:30
@cgillum
Copy link
Member

cgillum commented Aug 10, 2022

Looking at the original issue and looking at the test code for this PR, I was surprised that the analyzer was flagging parameters for methods that aren't functions. My expectation was that the analyzer would only consider function parameters -i.e., parameters in methods that have the [FunctionName] attribute.

Would that be another way to solve this problem?

@amdeel
Copy link
Contributor Author

amdeel commented Aug 10, 2022

@cgillum It's flagging the attributes because it's a method called in the orchetrator, so it's considered deterministic code. Do you think I should be checking code on orchestrator functions only for this analyzer instead?

@cgillum
Copy link
Member

cgillum commented Aug 10, 2022

I was just wondering why the analyzer would check method parameters on arbitrary methods. It wasn't clear to me how method parameters relate to orchestrator code constraints. I was assuming that this particular check was looking for illegal binding parameters, which are only relevant in function signatures.

@amdeel
Copy link
Contributor Author

amdeel commented Aug 10, 2022

@cgillum The deterministic analyzers currently analyze all orchestrator methods and methods called in orchestrators. I can just add a check on this analyzer specifically to prevent it from flagging code in methods without the FunctionName parameter on it, if you agree with that solution.

@cgillum
Copy link
Member

cgillum commented Aug 10, 2022

I can just add a check on this analyzer specifically to prevent it from flagging code in methods without the FunctionName parameter on it

This sounds great to me. This approach also helps ensure that we don't accidentally flag other parameter attributes, including custom parameter attributes (those written by the user and not provided by the framework) that we couldn't possibly know about ahead of time.

@amdeel amdeel changed the title Analyzer: Excluded null-state attributes from BindingAnalyzer Analyzer: Excluded non-Function methods from BindingAnalyzer Aug 11, 2022
@amdeel amdeel merged commit 7c7e640 into dev Aug 11, 2022
@amdeel amdeel deleted the AnalyzerNullAtrribute branch August 11, 2022 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants