Skip to content

[Linter]: Support for deprecated arguments - #2630

Open
gigalasr wants to merge 22 commits into
mainfrom
1870-deprecated-args-support
Open

[Linter]: Support for deprecated arguments#2630
gigalasr wants to merge 22 commits into
mainfrom
1870-deprecated-args-support

Conversation

@gigalasr

@gigalasr gigalasr commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

closes #1870

TODO:

  • Support for ifValue for args
  • Support version constraint
  • Support for Identifiers with package name

@gigalasr gigalasr self-assigned this Jul 9, 2026
Comment thread src/linter/rules/deprecated-functions.ts Outdated
@gigalasr

gigalasr commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Questions:

  1. Should the linter rule return the node id and source range of the entire function, or just of the deprecated arg?
  2. How can I make this work with the new identifiers that are prefixed with the package name?

@EagleoutIce

Copy link
Copy Markdown
Member
1. Should the linter rule return the node id and source range of the entire function, or just of the deprecated arg?

Just for the argument please

2. How can I make this work with the new identifiers that are prefixed with the package name?

unsure what you mean here because the argument names cannot be namespaced? Could you please elaborate with an example?

@gigalasr

gigalasr commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author
  1. Oki, thank you!
  2. In line 76 (https://github.com/flowr-analysis/flowr/pull/2630/changes#diff-08ba3901670b25d88a746877af089e407a93f5ec8a0b09db4b1bf47e1eb71c4aR76) I use the target from the CallTargetsEnrichment to index the whenArgs record. However, target can have names like deply:someFn, but the whenArgs record (currently) only contains names like someFn.

@EagleoutIce

Copy link
Copy Markdown
Member

Aah okay so you can use either Identifier::toQualified or also Identifier::matches (or match? not sure rn) to bridge the gap - is this what you needed?

@gigalasr

gigalasr commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Sounds like it, I will try that! Thank you!

Comment thread src/linter/rules/deprecated-functions.ts Outdated
Comment thread src/linter/rules/deprecated-functions.ts Outdated
Comment thread src/linter/rules/deprecated-functions.ts
Comment thread src/linter/rules/deprecated-functions.ts Outdated
Comment thread src/linter/rules/deprecated-functions.ts Outdated
Comment thread src/linter/rules/deprecated-functions.ts Outdated
Comment thread src/linter/rules/deprecated-functions.ts Outdated
Comment thread src/linter/rules/deprecated-functions.ts Outdated
Comment thread src/linter/rules/deprecated-functions.ts Outdated
Comment thread src/linter/rules/deprecated-functions.ts
@EagleoutIce
EagleoutIce force-pushed the main branch 2 times, most recently from db3f78d to e515c3b Compare August 1, 2026 08:46
@gigalasr
gigalasr marked this pull request as ready for review August 7, 2026 15:37
@gigalasr
gigalasr requested a review from EagleoutIce August 7, 2026 15:37
hardcoded: number
}

const AlwaysDeprecated = [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we map these to the built in call props

'all_equal', 'arrange_all', 'distinct_all', 'filter_all', 'group_by_all', 'summarise_all', 'mutate_all', 'select_all', 'vars', 'all_vars', 'id', 'failwith', 'select_vars', 'rename_vars', 'select_var', 'current_vars', 'bench_tbls', 'compare_tbls', 'compare_tbls2', 'eval_tbls', 'eval_tbls2', 'location', 'changes', 'combine', 'do', 'funs', 'add_count_', 'add_tally_', 'arrange1_', 'count_', 'distinct_', 'do_', 'filter_', 'funs_', 'group_by_', 'group_indices_', 'mutate_', 'tally_', 'transmute_', 'rename_', 'rename_vars_', 'select_', 'select_vars_', 'slice_', 'summarise_', 'summarize_', 'summarise_each', 'src_local', 'tbl_df', 'add_rownames', 'group_nest', 'group_split', 'with_groups', 'nest_by', 'progress_estimated', 'recode', 'sample_n', 'top_n', 'transmute', 'fct_explicit_na', 'aes_', 'aes_auto', 'annotation_logticks', 'is.Coord', 'coord_flip', 'coord_map', 'is.facet', 'fortify', 'is.ggproto', 'guide_train', 'is.ggplot', 'qplot', 'is.theme', 'gg_dep', 'liply', 'isplit2', 'list_along', 'cross', 'invoke', 'at_depth', 'prepend', 'rerun', 'splice', '`%@%`', 'rbernoulli', 'rdunif', 'when', 'update_list', 'map_raw', 'accumulate', 'reduce_right', 'flatten', 'map_dfr', 'as_vector', 'transpose', 'melt_delim', 'melt_fwf', 'melt_table', 'read_table2', 'str_interp', 'as_tibble', 'data_frame', 'tibble_', 'data_frame_', 'lst_', 'as_data_frame', 'as.tibble', 'frame_data', 'trunc_mat', 'is.tibble', 'tidy_names', 'set_tidy_names', 'repair_names', 'extract_numeric', 'complete_', 'drop_na_', 'expand_', 'crossing_', 'nesting_', 'extract_', 'fill_', 'gather_', 'nest_', 'separate_rows_', 'separate_', 'spread_', 'unite_', 'unnest_', 'extract', 'gather', 'nest_legacy', 'separate_rows', 'separate', 'spread'
];

const ConditionallyDeprecated = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice as an example :) maybe reference the deprecation notice/url?

export interface DeprecatedArgumentResult extends DeprecatedFunctionResultBase {
readonly type: 'deprecated-argument'
/** The name or index of the deprecated argument */
readonly arg: string | number

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this does not make it clear what this will contain. like why allow both? every parameter has a name so there should be one. is this automatically a name if it was given named and index if unnamed?

/** A better alternative is available, and the function is marked for removal {@link https://lifecycle.r-lib.org/articles/stages.html#deprecated} */
Deprecated = 'deprecated',
/** No longer works and is removed and replaced by another function {@link https://www.rdocumentation.org/packages/base/versions/3.6.2/topics/Defunct} */
Defunct = 'defunct'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is defunct associated with exceptions? if so we could pipe this into flowR's exception handling

}

interface Metadata extends MergeableRecord {
sigdb: number,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

does this code the number of hits from the sigdb, the number of entries in the sigdb.... this should be made clearer

*/
async function deprecateFunctionConditionally(candidate: PotentialFunction, dataflow: DataflowGraph, idMap: AstIdMap, analyzer: ReadonlyFlowrAnalysisProvider<KnownParser>, info: DeprecatedFunctionInformation): Promise<DeprecatedFunctionRuleResult[]> {
const results: DeprecatedFunctionRuleResult[] = [];
const result = await analyzer.query([{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

uuuh this is a performance hazard. check whether you need to do this first, then do it once and take the info. even if you guess just one package this has to solve a big system.

const name = candidate.target.includes('::') ? Identifier.getName(Identifier.parse(candidate.target)) : candidate.target;
const info = config.conditionally[name];
if(isNotUndefined(info)) {
// Check functions from DeprecatedFunctionsConfig.conditionally

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

any reasons for these comments?


// 2. Uses hardcoded information about deprecated arguments and deprecated functions
const results: DeprecatedFunctionRuleResult[] = (await Promise.all(detectedFunctions.map(async candidate => {
const name = candidate.target.includes('::') ? Identifier.getName(Identifier.parse(candidate.target)) : candidate.target;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why even check for :: Identifier shuold be doing this just fine! I nmean its technically fine as a safe guard but this could go south


// Deprecated Argument: If `whenArgs` is provided, only mark deprecated arguments
if(info.whenArgs) {
const vertex = dataflow.getVertex(candidate.node.info.id);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could we please structure this into helper functions? :D

/**
* Test if a function call has an argument with a specific value
*/
export function hasArgumentValue(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this could be.. should be moved in to the Rargument helper object i think

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.

[Linter] Deprecated arguments support

2 participants