Skip to content

chore: add no-unused-expressions lint rule #7007

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.

Sign up for GitHub

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

chore: add no-unused-expressions lint rule #7007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
aede19c
Update eslint.config.mjs
simon-id Nov 27, 2025
42e153b
zeui
simon-id Nov 27, 2025
1ec0138
add void before error.stack
simon-id Dec 1, 2025
41f60e0
replace short-circuit with optional chaining
simon-id Dec 1, 2025
4586202
replace short-circuit with if condition
simon-id Dec 1, 2025
c851f95
add probably missing return
simon-id Dec 1, 2025
7ab17ec
order rule correctly
simon-id Dec 1, 2025
2182259
Merge branch 'master' into simon-id-patch-2
simon-id Dec 1, 2025
511df85
disable no-void
simon-id Dec 1, 2025
5782f36
fix weird line
simon-id Dec 1, 2025
61593c8
Update packages/dd-trace/src/remote_config/scheduler.js
simon-id Dec 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions eslint.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,9 @@ export default [
'no-await-in-loop': 'error',
'no-else-return': ['error', { allowElseIf: true }],
'no-implicit-coercion': ['error', { boolean: true, number: true, string: true, allow: ['!!'] }],
'no-unused-expressions': 'error',
'no-useless-assignment': 'error',
'no-void': 'off',
Copy link
Collaborator

@BridgeAR BridgeAR Dec 2, 2025

Choose a reason for hiding this comment

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

I think it is better to use the eslint exception for accessing the stack, while I am uncertain if accessing it is still needed at all. There were some V8 changes that might make that obsolete (I did not check).

Copy link
Member Author

@simon-id simon-id Dec 2, 2025

Choose a reason for hiding this comment

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

any reasoning against void ? just picked that because i never remember the eslint exception syntax, void is easy to remember

'operator-assignment': 'error',
'prefer-exponentiation-operator': 'error',
'prefer-object-has-own': 'error',
Expand Down
6 changes: 3 additions & 3 deletions packages/datadog-instrumentations/src/couchbase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ function wrap (prefix, fn) {
return fn.apply(this, arguments)
} catch (error) {
ctx.error = error
error.stack // trigger getting the stack at the original throwing point
void error.stack // trigger getting the stack at the original throwing point
errorCh.publish(ctx)

throw error
Expand Down Expand Up @@ -162,7 +162,7 @@ function wrapCBandPromise (fn, name, startData, thisArg, args) {
)
return res
} catch (e) {
e.stack
void e.stack
ctx.error = e
errorCh.publish(ctx)
throw e
Expand Down Expand Up @@ -226,7 +226,7 @@ addHook({ name: 'couchbase', file: 'lib/bucket.js', versions: ['^2.6.12'] }, Buc
try {
return _n1qlReq.apply(this, arguments)
} catch (err) {
err.stack // trigger getting the stack at the original throwing point
void err.stack // trigger getting the stack at the original throwing point
ctx.error = err
errorCh.publish(ctx)

Expand Down
2 changes: 1 addition & 1 deletion packages/datadog-instrumentations/src/dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ function wrap (prefix, fn, expectedArgs, rrtype) {
return fn.apply(this, arguments)
// TODO deal with promise versions when we support `dns/promises`
} catch (error) {
error.stack // trigger getting the stack at the original throwing point
void error.stack // trigger getting the stack at the original throwing point
ctx.error = error
errorCh.publish(ctx)

Expand Down
2 changes: 1 addition & 1 deletion packages/datadog-instrumentations/src/elasticsearch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ function createWrapRequest (name) {
}
return promise
} catch (err) {
err.stack // trigger getting the stack at the original throwing point
void err.stack // trigger getting the stack at the original throwing point
errorCh.publish(err)

throw err
Expand Down
4 changes: 2 additions & 2 deletions packages/datadog-instrumentations/src/graphql.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ function wrapParse (parse) {

return ctx.document
} catch (err) {
err.stack
void err.stack
ctx.error = err
parseErrorCh.publish(ctx)

Expand Down Expand Up @@ -132,7 +132,7 @@ function wrapValidate (validate) {
}
return errors
} catch (err) {
err.stack
void err.stack
ctx.error = err
validateErrorCh.publish(ctx)

Expand Down
2 changes: 1 addition & 1 deletion packages/datadog-instrumentations/src/mariadb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ function createWrapQuery (options) {
finishCh.publish(ctx)
return result
}, error => {
ctx.error
ctx.error = error
Copy link
Member Author

@simon-id simon-id Dec 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@rochdev need your approval on this, the change i made is only a guess

Copy link
Collaborator

@BridgeAR BridgeAR Dec 2, 2025

Choose a reason for hiding this comment

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

This looks like a bug fix. Could you please add a test for that?

Copy link
Member Author

@simon-id simon-id Dec 2, 2025

Choose a reason for hiding this comment

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

yes, but i'll let roch do this, because i don't even know what bug this fixes

errorCh.publish(ctx)
finishCh.publish(ctx)
throw error
Expand Down
2 changes: 1 addition & 1 deletion packages/datadog-instrumentations/src/mysql.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ addHook({ name: 'mysql', file: 'lib/Connection.js', versions: ['>=2'] }, Connect

return res
} catch (err) {
err.stack // trigger getting the stack at the original throwing point
void err.stack // trigger getting the stack at the original throwing point
ctx.error = err
errorCh.publish(ctx)

Expand Down
2 changes: 1 addition & 1 deletion packages/datadog-plugin-azure-functions/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ function extractTraceContext (tracer, ctx) {
case 'Http':
return tracer.extract('http_headers', Object.fromEntries(ctx.httpRequest.headers))
default:
null
return null
Copy link
Collaborator

@BridgeAR BridgeAR Dec 2, 2025

Choose a reason for hiding this comment

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

AFAIK it's even possible to just remove the default which would allow this to be a simple if statement. While being on this: the String() call is not needed.

Copy link
Member Author

@simon-id simon-id Dec 2, 2025

Choose a reason for hiding this comment

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

Of course the default here is not needed if the caller accept undefined instead of null, but i'm trying to keep the changes as iso as possible, this is just a linter PR, if i stop at every little thing that can be improved, the PR will be very off topic

Copy link
Member Author

@simon-id simon-id Dec 2, 2025

Choose a reason for hiding this comment

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

I thought it was unnecessary when i saw it too, but if i go and change it, i run the risk of creating more problems and failing tests and stuff, that i'm not interested in investing time on now. Especially since I'm making this PR completely online without running it locally, so iteration time is way slower. Small PR for a small task

Copy link
Collaborator

@watson watson Dec 3, 2025

Choose a reason for hiding this comment

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

Since it didn't return null before, but undefined, I'd argue that there's a bigger risk making this change vs just removing the entire default statement. If you're unsure, look at how the caller handles the return value.

Copy link
Member Author

@simon-id simon-id Dec 3, 2025

Choose a reason for hiding this comment

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

Very good point, you convinced me, I'll make it a if and ping whoever wrote this

}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/dd-trace/src/opentracing/propagation/text_map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ class TextMapPropagator {
}

_injectBaggageItems (spanContext, carrier) {
if (this._config.legacyBaggageEnabled) {
spanContext?._baggageItems && Object.keys(spanContext._baggageItems).forEach(key => {
if (this._config.legacyBaggageEnabled && spanContext?._baggageItems) {
Object.keys(spanContext._baggageItems).forEach(key => {
carrier[baggagePrefix + key] = String(spanContext._baggageItems[key])
})
}
Expand Down
2 changes: 1 addition & 1 deletion packages/dd-trace/src/remote_config/scheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class Scheduler {
runAfterDelay (interval = this._interval) {
this._timer = setTimeout(this._callback, interval, () => this.runAfterDelay())

this._timer.unref && this._timer.unref()
this._timer.unref()
}

stop () {
Expand Down