Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.
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
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,8 @@ const zigSpec: LanguageSpec = {
*
* The set of languages come from https://madnight.github.io/githut/#/pull_requests/2018/4 (with additions)
* The language names come from https://code.visualstudio.com/docs/languages/identifiers#_known-language-identifiers.
*
* @deprecated See FIXME(id: language-detection)
*/
export const languageSpecs: LanguageSpec[] = [
apexSpec,
Expand Down Expand Up @@ -425,6 +427,8 @@ export const languageSpecs: LanguageSpec[] = [
/**
* Returns the language spec with the given language identifier. If no language
* matches is configured with the given identifier an error is thrown.
*
* @deprecated See FIXME(id: language-detection)
*/
export function findLanguageSpec(languageID: string): LanguageSpec | undefined {
return languageSpecs.find(spec => spec.languageID === languageID || spec.additionalLanguages?.includes(languageID))
Expand Down
17 changes: 17 additions & 0 deletions client/shared/src/languages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,22 @@ import { basename } from 'path'
/** The LSP mode used for plain text files and all other unrecognized files. */
const PLAINTEXT_MODE = 'plaintext'

// FIXME(id: language-detection)
//
// For correctness, we should do language detection on the server
// and use the inferred language on the client instead of incorrectly
// doing language detection on the client using incomplete information
// like file extensions.
//
// For example, MATLAB and Objective-C both use the '.m' file extension.
//
// See https://github.com/sourcegraph/sourcegraph/issues/56376

/**
* getModeFromPath returns the LSP mode for the provided file path. If the file path does not correspond to any
* known mode, 'plaintext' is returned.
*
* @deprecated See FIXME(id: language-detection).
*/
export function getModeFromPath(path: string): string {
const fileName = basename(path)
Expand All @@ -19,6 +32,8 @@ export function getModeFromPath(path: string): string {
* provided file name (e.g. "dockerfile")
*
* Cherry picked from https://github.com/github/linguist/blob/master/lib/linguist/languages.yml
*
* @deprecated See FIXME(id: language-detection)
*/
function getModeFromExactFilename(fileName: string): string | undefined {
switch (fileName.toLowerCase()) {
Expand All @@ -43,6 +58,8 @@ function getModeFromExactFilename(fileName: string): string | undefined {
*
* Cherry picked from https://github.com/isagalaev/highlight.js/tree/master/src/languages
* and https://github.com/github/linguist/blob/master/lib/linguist/languages.yml.
*
* @deprecated See FIXME(id: language-detection)
*/
function getModeFromExtension(extension: string): string | undefined {
switch (extension.toLowerCase()) {
Expand Down
83 changes: 47 additions & 36 deletions client/web/src/codeintel/ReferencesPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { type Observable, of } from 'rxjs'
import { map } from 'rxjs/operators'

import { CodeExcerpt } from '@sourcegraph/branded'
import type { HoveredToken } from '@sourcegraph/codeintellify'
import { type ErrorLike, logger, pluralize } from '@sourcegraph/common'
import { Position } from '@sourcegraph/extension-api-classes'
import type { FetchFileParameters } from '@sourcegraph/shared/src/backend/file'
Expand Down Expand Up @@ -56,13 +55,12 @@ import type { CodeIntelligenceProps } from '.'
import { type Location, LocationsGroup, type LocationsGroupedByRepo, type LocationsGroupedByFile } from './location'
import { newSettingsGetter } from './settings'
import { SideBlob, type SideBlobProps } from './SideBlob'
import { findSearchToken } from './token'
import { findSearchToken, type ZeroBasedPosition } from './token'
import { useRepoAndBlob } from './useRepoAndBlob'
import { isDefined } from './util/helpers'

import styles from './ReferencesPanel.module.scss'

type Token = HoveredToken & RepoSpec & RevisionSpec & FileSpec & ResolvedRevisionSpec
type Token = { range: State['range'] } & RepoSpec & RevisionSpec & FileSpec & ResolvedRevisionSpec

interface HighlightedFileLineRangesProps {
fetchHighlightedFileLineRanges: (parameters: FetchFileParameters, force?: boolean) => Observable<string[][]>
Expand All @@ -88,8 +86,10 @@ interface State {
repoName: string
revision?: string
filePath: string
line: number
character: number
range: {
start: OneBasedPosition
end?: OneBasedPosition
}
jumpToFirst: boolean
collapsedState: {
references: boolean
Expand All @@ -99,9 +99,14 @@ interface State {
}
}

interface OneBasedPosition {
line: number
character: number
}
Comment on lines +102 to +105
Copy link
Contributor

Choose a reason for hiding this comment

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

I've also run into quite a few issues with 0-based vs 1-based positions. I think it's useful to have separate types to express intent, but just note that the type checker won't prevent you from passing a ZeroBasedPostition as a OneBasedPosition, because TS uses structural equivalence (or whatever it is called).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think I should use classes here? I'd be happy to introduce new vocabulary types for Positions and Ranges in a central place that can be reused elsewhere.

Copy link
Contributor Author

@varungandhi-src varungandhi-src Dec 14, 2023

Choose a reason for hiding this comment

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

OK, let me attempt to do that in a follow-up PR. Thanks for flagging this, I forgot that interfaces have structural subtyping.


function createStateFromLocation(location: H.Location): null | State {
const { hash, pathname, search } = location
const { line, character, viewState } = parseQueryAndHash(search, hash)
const { line, character, endLine, endCharacter, viewState } = parseQueryAndHash(search, hash)
const { filePath, repoName, revision } = parseBrowserRepoURL(pathname)

// If we don't have enough information in the URL, we can't render the panel
Expand Down Expand Up @@ -129,7 +134,11 @@ function createStateFromLocation(location: H.Location): null | State {
collapsedState.definitions = true
}

return { repoName, revision, filePath, line, character, jumpToFirst, collapsedState }
const range = {
start: { line, character },
end: endLine && endCharacter ? { line: endLine, character: endCharacter } : undefined,
}
return { repoName, revision, filePath, range, jumpToFirst, collapsedState }
}

export const ReferencesPanel: React.FunctionComponent<React.PropsWithChildren<ReferencesPanelProps>> = props => {
Expand All @@ -146,8 +155,7 @@ export const ReferencesPanel: React.FunctionComponent<React.PropsWithChildren<Re
repoName={state.repoName}
revision={state.revision}
filePath={state.filePath}
line={state.line}
character={state.character}
range={state.range}
jumpToFirst={state.jumpToFirst}
collapsedState={state.collapsedState}
/>
Expand All @@ -158,8 +166,7 @@ const RevisionResolvingReferencesList: React.FunctionComponent<
React.PropsWithChildren<
ReferencesPanelProps & {
repoName: string
line: number
character: number
range: State['range']
filePath: string
revision?: string
collapsedState: State['collapsedState']
Expand All @@ -171,7 +178,7 @@ const RevisionResolvingReferencesList: React.FunctionComponent<
// Scroll blob UI to the selected symbol right after the reference panel is rendered
// and shifted the blob UI (scroll into view is needed because there are a few cases
// when ref panel may overlap with current symbol)
useEffect(() => BlobAPI.scrollIntoView({ line: props.line }), [props.line])
useEffect(() => BlobAPI.scrollIntoView({ line: props.range.start.line }), [props.range.start.line])

if (loading && !data) {
return <LoadingCodeIntel />
Expand All @@ -192,8 +199,7 @@ const RevisionResolvingReferencesList: React.FunctionComponent<

const token = {
repoName: props.repoName,
line: props.line,
character: props.character,
range: props.range,
filePath: props.filePath,
revision: data.revision,
commitID: data.commitID,
Expand All @@ -220,29 +226,28 @@ interface ReferencesPanelPropsWithToken extends ReferencesPanelProps {
collapsedState: State['collapsedState']
}

function oneBasedPositionToZeroBased(p: OneBasedPosition): ZeroBasedPosition {
return {
line: p.line - 1,
character: p.character - 1,
}
}

const SearchTokenFindingReferencesList: React.FunctionComponent<
React.PropsWithChildren<ReferencesPanelPropsWithToken>
> = props => {
const languageId = getModeFromPath(props.token.filePath)
const spec = findLanguageSpec(languageId)
const tokenResult =
spec &&
findSearchToken({
text: props.fileContent,
position: {
line: props.token.line - 1,
character: props.token.character - 1,
},
lineRegexes: spec.commentStyles.map(style => style.lineRegex).filter(isDefined),
blockCommentStyles: spec.commentStyles.map(style => style.block).filter(isDefined),
identCharPattern: spec.identCharPattern,
})
const tokenRange = props.token.range
const tokenResult = findSearchToken({
text: props.fileContent,
start: oneBasedPositionToZeroBased(tokenRange.start),
end: tokenRange.end ? oneBasedPositionToZeroBased(tokenRange.end) : undefined,
})
const shouldMixPreciseAndSearchBasedReferences: boolean = newSettingsGetter(props.settingsCascade)<boolean>(
'codeIntel.mixPreciseAndSearchBasedReferences',
false
)

if (!spec || !tokenResult?.searchToken) {
if (tokenResult === undefined) {
return (
<div>
<Text className="text-danger">Could not find token.</Text>
Expand All @@ -256,8 +261,13 @@ const SearchTokenFindingReferencesList: React.FunctionComponent<
// change. This way we avoid showing stale results.
key={shouldMixPreciseAndSearchBasedReferences.toString()}
{...props}
searchToken={tokenResult?.searchToken}
spec={spec}
searchToken={tokenResult}
// The file extensions attached to the 'spec' value here are
// used for search-based code intel. However, determining
// the spec purely based on the file path is wrong.
//
// See FIXME(id: language-detection).
spec={findLanguageSpec(getModeFromPath(props.token.filePath))}
/>
)
}
Expand All @@ -268,7 +278,7 @@ const ReferencesList: React.FunctionComponent<
React.PropsWithChildren<
ReferencesPanelPropsWithToken & {
searchToken: string
spec: LanguageSpec
spec: LanguageSpec | undefined
fileContent: string
collapsedState: State['collapsedState']
}
Expand Down Expand Up @@ -305,8 +315,8 @@ const ReferencesList: React.FunctionComponent<
path: props.token.filePath,
// On the backend the line/character are 0-indexed, but what we
// get from hoverifier is 1-indexed.
line: props.token.line - 1,
character: props.token.character - 1,
line: props.token.range.start.line - 1,
character: props.token.range.start.character - 1,
filter: debouncedFilter || null,
firstReferences: 100,
afterReferences: null,
Expand Down Expand Up @@ -993,7 +1003,8 @@ const LoadingCodeIntelFailed: React.FunctionComponent<React.PropsWithChildren<{
)

function sessionStorageKeyFromToken(token: Token): string {
return `${token.repoName}@${token.commitID}/${token.filePath}?L${token.line}:${token.character}`
const start = token.range.start
return `${token.repoName}@${token.commitID}/${token.filePath}?L${start.line}:${start.character}`
}

function locationToUrl(location: Location): string {
Expand Down
Loading