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

Add strict type-checks that most switch statements cover all cases #477

Merged
merged 2 commits into from
Jun 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .changelog/477.internal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add strict type-checks that most switch statements cover all cases
8 changes: 8 additions & 0 deletions src/app/components/Transactions/LogEvent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { TransactionLink } from './TransactionLink'
import { SearchScope } from '../../../types/searchScope'
import { AddressSwitchOption } from '../AddressSwitch'
import { getOasisAddress } from '../../utils/helpers'
import { exhaustedTypeWarning } from '../../../types/errors'

const EvmEventParamData: FC<{
scope: SearchScope
Expand Down Expand Up @@ -155,7 +156,14 @@ const DecodedLogEvent: FC<{
case RuntimeEventType.accountstransfer:
case RuntimeEventType.consensus_accountsdeposit:
case RuntimeEventType.consensus_accountswithdraw:
return (
<div>
<div>{eventName}</div>
<pre>{JSON.stringify(event, null, ' ')}</pre>
</div>
)
default:
exhaustedTypeWarning('Unexpected event type', event.type)
return (
<div>
<div>{eventName}</div>
Expand Down
7 changes: 6 additions & 1 deletion src/app/pages/DashboardPage/LearningMaterials.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import Typography from '@mui/material/Typography'
import { styled } from '@mui/material/styles'
import { COLORS } from '../../../styles/theme/colors'
import { docs } from '../../utils/externalLinks'
import { AppError, AppErrors } from '../../../types/errors'
import { AppError, AppErrors, exhaustedTypeWarning } from '../../../types/errors'
import { Layer } from '../../../oasis-indexer/api'
import { useRequiredScopeParam } from '../../hooks/useScopeParam'

Expand Down Expand Up @@ -49,7 +49,12 @@ const getContent = (t: TFunction, layer: Layer) => {
link: docs.sapphire,
title: t('common.sapphire'),
}
case Layer.cipher:
throw new AppError(AppErrors.UnsupportedLayer)
case Layer.consensus:
throw new AppError(AppErrors.UnsupportedLayer)
default:
exhaustedTypeWarning('Unexpected layer', layer)
throw new AppError(AppErrors.UnsupportedLayer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we are throwing two exceptions on quick succession. (At least in PROD mode.) Are they both necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, unless we introduce more fallbacks

}
}
Expand Down
3 changes: 3 additions & 0 deletions src/app/pages/HomePage/Graph/Graph/graph-utils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { ScaleToOptions } from 'react-quick-pinch-zoom'
import { Layer } from '../../../../../oasis-indexer/api'
import { exhaustedTypeWarning } from '../../../../../types/errors'

export abstract class GraphUtils {
static getScaleTo(layer: Layer, { width, height }: { width?: number; height?: number }): ScaleToOptions {
Expand Down Expand Up @@ -33,7 +34,9 @@ export abstract class GraphUtils {
y: height,
}
case Layer.consensus:
return initialValue
default:
exhaustedTypeWarning('Unexpected layer', layer)
return initialValue
}
}
Expand Down
5 changes: 5 additions & 0 deletions src/app/pages/HomePage/Graph/para-time-selector-utils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { ParaTimeSelectorStep } from './types'
import { Layer } from '../../../../oasis-indexer/api'
import { exhaustedTypeWarning } from '../../../../types/errors'

export abstract class ParaTimeSelectorUtils {
static getIsGraphTransparent(step: ParaTimeSelectorStep) {
Expand Down Expand Up @@ -28,7 +29,11 @@ export abstract class ParaTimeSelectorUtils {
case Layer.emerald:
case Layer.cipher:
return true
case Layer.consensus:
case undefined:
return false
default:
exhaustedTypeWarning('Unexpected layer', layer)
return false
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { SearchResults } from './hooks'
import { RouteUtils } from '../../utils/route-utils'
import { isItemInScope, SearchScope } from '../../../types/searchScope'
import { Network } from '../../../types/network'
import { exhaustedTypeWarning } from '../../../types/errors'

/** If search only finds one result then redirect to it */
export function useRedirectIfSingleResult(scope: SearchScope | undefined, results: SearchResults) {
Expand All @@ -30,11 +31,7 @@ export function useRedirectIfSingleResult(scope: SearchScope | undefined, result
redirectTo = RouteUtils.getAccountRoute(item, item.address_eth ?? item.address)
break
default:
// The conversion of any is necessary here, since we have covered all possible subtype,
// and TS is concluding that the only possible remaining type is "never".
// However, if we all more result types in the future and forget to add the appropriate case here,
// we might hit this, hence the warning.
console.log(`Don't know how to redirect to unknown search result type ${(item as any).resultType}`)
exhaustedTypeWarning('Unexpected result type', item)
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/coin-gecko/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { useQuery } from '@tanstack/react-query'
import { getTickerForNetwork, NativeTicker, Ticker } from '../types/ticker'
import { Network } from '../types/network'
import { RouteUtils } from '../app/utils/route-utils'
import { exhaustedTypeWarning } from '../types/errors'

type GetRosePriceParams = {
ids: string
Expand Down Expand Up @@ -68,7 +69,7 @@ export const useTokenPrice = (ticker: NativeTicker): TokenPriceInfo => {
isFree: true,
}
default:
console.warn('Checking price of unknown token', ticker)
exhaustedTypeWarning('Checking price of unknown token', ticker)
return {
isLoading: false,
hasUsedCoinGecko: false,
Expand Down
17 changes: 17 additions & 0 deletions src/types/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,20 @@ export interface ErrorPayload {
code: AppErrors
message: string
}

// Adds strict type-check that a type was exhausted
// https://www.typescriptlang.org/docs/handbook/2/narrowing.html#exhaustiveness-checking
// https://stackoverflow.com/questions/41102060/typescript-extending-error-class
export async function exhaustedTypeWarning(
messagePrefix: string,
exhaustedType: 'Expected type to be exhausted, but this type was not handled',
) {
const message = `${messagePrefix}: Expected type to be exhausted, but this type was not handled: ${JSON.stringify(
exhaustedType,
)}`
if (process.env.NODE_ENV === 'production') {
console.warn(message)
} else {
throw new Error(message)
}
}
2 changes: 1 addition & 1 deletion src/types/ticker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export type NativeTicker = (typeof Ticker)[keyof typeof Ticker]
export const Ticker = {
ROSE: 'ROSE',
TEST: 'TEST',
}
} as const

const networkTicker: Record<Network, NativeTicker> = {
[Network.mainnet]: Ticker.ROSE,
Expand Down